Re: [Qemu-devel] [PATCH v3 4/4] ivshmem: Disable irqfd on device reset
Ladi Prosek writes:
> On Fri, Dec 8, 2017 at 6:28 PM, Markus Armbruster wrote:
>> Ladi Prosek writes:
>>
>>> On Fri, Dec 8, 2017 at 2:36 PM, Markus Armbruster wrote:
Ladi Prosek writes:
> The effects of ivshmem_enable_irqfd() was not undone on device reset.
>
> This manifested as:
> ivshmem_add_kvm_msi_virq: Assertion `!s->msi_vectors[vector].pdev' failed.
>
> when irqfd was enabled before reset and then enabled again after reset,
> making
> ivshmem_enable_irqfd() run for the second time.
>
> To reproduce, run:
>
> ivshmem-server
>
> and QEMU with:
>
> -device ivshmem-doorbell,chardev=iv
> -chardev socket,path=/tmp/ivshmem_socket,id=iv
>
> then install the Windows driver, at the time of writing available at:
>
> https://github.com/virtio-win/kvm-guest-drivers-windows/tree/master/ivshmem
>
> and crash-reboot the guest by inducing a BSOD.
>
> Signed-off-by: Ladi Prosek
> ---
> hw/misc/ivshmem.c | 24 +---
> 1 file changed, 13 insertions(+), 11 deletions(-)
>
> diff --git a/hw/misc/ivshmem.c b/hw/misc/ivshmem.c
> index d1bb246d12..4be0d2627b 100644
> --- a/hw/misc/ivshmem.c
> +++ b/hw/misc/ivshmem.c
> @@ -758,17 +758,6 @@ static void ivshmem_msix_vector_use(IVShmemState *s)
> }
> }
>
> -static void ivshmem_reset(DeviceState *d)
> -{
> -IVShmemState *s = IVSHMEM_COMMON(d);
> -
> -s->intrstatus = 0;
> -s->intrmask = 0;
> -if (ivshmem_has_feature(s, IVSHMEM_MSI)) {
> -ivshmem_msix_vector_use(s);
> -}
> -}
> -
> static int ivshmem_setup_interrupts(IVShmemState *s, Error **errp)
> {
> /* allocate QEMU callback data for receiving interrupts */
> @@ -855,6 +844,19 @@ static void ivshmem_disable_irqfd(IVShmemState *s)
>
> }
>
> +static void ivshmem_reset(DeviceState *d)
> +{
> +IVShmemState *s = IVSHMEM_COMMON(d);
> +
> +ivshmem_disable_irqfd(s);
> +
> +s->intrstatus = 0;
> +s->intrmask = 0;
> +if (ivshmem_has_feature(s, IVSHMEM_MSI)) {
> +ivshmem_msix_vector_use(s);
> +}
> +}
> +
> static void ivshmem_write_config(PCIDevice *pdev, uint32_t address,
> uint32_t val, int len)
> {
Why are you moving ivshmem_reset()? Makes the actual change harder to
see than necessary.
>>>
>>> ivshmem_disable_irqfd() is declared after ivshmem_reset() in the
>>> source file. I generally prefer to order static functions
>>> topologically if possible. If you'd prefer adding a forward decl
>>> instead (fewer lines touched, easier to bisect?) I can certainly do
>>> that. Thanks!
>>
>> Well, it compiles before your patch, your patch doesn't add any calls,
>> so I can't quite see why a forward declaration would be needed.
>
> It adds a call to ivshmem_disable_irqfd().
Note to self: patch review on Friday late afternoon is a bad idea.
I'd prefer the forward declaration. Thanks!
Re: [Qemu-devel] [PATCH v3 for-2.12 03/14] s390x/tcg: implement SET CLOCK PROGRAMMABLE FIELD
On 12/08/2017 08:01 AM, David Hildenbrand wrote: > Needed for machine check handling inside Linux (when restoring registers). > > Except for SIGP and machine checks, we don't make use of the register > yet. Sufficient for now. > > Reviewed-by: Thomas Huth > Signed-off-by: David Hildenbrand > --- > target/s390x/helper.h | 1 + > target/s390x/insn-data.def | 2 ++ > target/s390x/misc_helper.c | 11 +++ > target/s390x/translate.c | 7 +++ > 4 files changed, 21 insertions(+) Reviewed-by: Richard Henderson r~
Re: [Qemu-devel] [PATCH v3 for-2.12 01/14] s390x/kvm: factor out build_channel_report_mcic() into cpu.h
On 12/08/2017 08:01 AM, David Hildenbrand wrote: > We'll need it later on in two places. Refactor it to just indicate the > validity bits. While at it, introduce a define for the used CR14 bit (we'll > also need later on). > > Signed-off-by: David Hildenbrand > --- > target/s390x/cpu.h | 23 +++ > target/s390x/kvm.c | 25 ++--- > 2 files changed, 25 insertions(+), 23 deletions(-) Reviewed-by: Richard Henderson r~
Re: [Qemu-devel] [PATCH 09/17] iotests: Disable some tests for compat=0.10
On 11/22/2017 09:08 PM, Max Reitz wrote: > Tests 080, 130, 137, and 176 simply do not work with compat=0.10 for the > reasons stated there. > > 177 is a bit more interesting: Originally, it was actually very much > intended to work with compat=0.10 (it even had a special case for that). > However, it now prints the test image's map twice, and short of just not > doing that, there is no solution I can imagine that is both simple and > would leave compat=0.10 support intact. > So we lost that support in f0a9c18f9e7 and 81c219ac6ce Eric, any input before we downscope your test? > Signed-off-by: Max Reitz Without agonizing over it, I don't see an easy win either, so: Reviewed-by: John Snow (but it is a shame to lose the ability to test it.)
Re: [Qemu-devel] [PATCH 08/17] iotests: Skip 103 for refcount_bits=1
On 11/30/2017 08:23 AM, Max Reitz wrote: > On 2017-11-30 04:18, Fam Zheng wrote: >> On Thu, 11/23 03:08, Max Reitz wrote: >>> Signed-off-by: Max Reitz >>> --- >>> tests/qemu-iotests/103 | 2 ++ >>> 1 file changed, 2 insertions(+) >>> >>> diff --git a/tests/qemu-iotests/103 b/tests/qemu-iotests/103 >>> index ecbd8ebd71..d0cfab8844 100755 >>> --- a/tests/qemu-iotests/103 >>> +++ b/tests/qemu-iotests/103 >>> @@ -40,6 +40,8 @@ trap "_cleanup; exit \$status" 0 1 2 3 15 >>> _supported_fmt qcow2 >>> _supported_proto file nfs >>> _supported_os Linux >>> +# Internal snapshots are (currently) impossible with refcount_bits=1 >>> +_unsupported_imgopts 'refcount_bits=1[^0-9]' >> >> What is the "[^0-9]" part for? > > It's so you can specify refcount_bits=16, but not > refcount_bits=1,compat=0.10 or just refcount_bits=1. > > Max > Worth a comment? Reviewed-by: John Snow
Re: [Qemu-devel] [PATCH 17/17] iotests: Filter compat-dependent info in 198
On 11/22/2017 09:08 PM, Max Reitz wrote: > There is a bit of image-specific information which depends on the qcow2 > compat level. Filter it so that 198 works with compat=0.10 (and any > refcount_bits value). > > Note that we cannot simply drop the --format-specific switch because we > do need the "encrypt" information. > I imagine it's not worth your time to factor this out. That makes sense to me. > Signed-off-by: Max Reitz Reviewed-by: John Snow
Re: [Qemu-devel] [PATCH 16/17] iotests: Make 191 work with qcow2 options
On 11/22/2017 09:08 PM, Max Reitz wrote: > In order for 191 to work with an explicit refcount_bits or compat=0.10, > we should strip format-specific information from the output--and we can > do so by using _filter_img_info. > > Signed-off-by: Max Reitz > --- > tests/qemu-iotests/191 | 5 +- "Looks good!" > tests/qemu-iotests/191.out | 313 > + * just runs the test instead * Reviewed-by: John Snow
Re: [Qemu-devel] [PATCH 15/17] iotests: Make 184 image-less
On 11/22/2017 09:08 PM, Max Reitz wrote: > 184 does not need an image, so don't use one. > > Signed-off-by: Max Reitz Seems sane. Reviewed-by: John Snow > --- > tests/qemu-iotests/184 | 25 -- > tests/qemu-iotests/184.out | 63 > +++--- > 2 files changed, 14 insertions(+), 74 deletions(-) >
Re: [Qemu-devel] [PATCH 1/6] hw/arm/virt: Check that the CPU realize method succeeded
On Thu, Dec 07, 2017 at 06:14:48PM +, Peter Maydell wrote: > We were passing a NULL error pointer to the object_property_set_bool() > call that realizes the CPU object. This meant that we wouldn't detect > failure, and would plough blindly on to crash later trying to use a > NULL CPU object pointer. Detect errors and fail instead. > > In particular, this will be necessary to detect the user error > of using "-cpu host" without "-enable-kvm" once we make the host > CPU type be registered unconditionally rather than only in > kvm_arch_init(). > > Signed-off-by: Peter Maydell Reviewed-by: Eduardo Habkost -- Eduardo
Re: [Qemu-devel] [PATCH 14/17] iotests: Make 089 compatible with compat=0.10
On 11/22/2017 09:08 PM, Max Reitz wrote: > The only thing that is missing is a _filter_img_info after the > "$QEMU_IO -c info" invocations. > > Signed-off-by: Max Reitz Reviewed-by: John Snow
Re: [Qemu-devel] [Qemu-arm] [PATCH 0/6] arm: support -cpu max (and gic-version=max)
On Thu, Dec 07, 2017 at 07:37:31PM +, Peter Maydell wrote: > On 7 December 2017 at 18:14, Peter Maydell wrote: > > This patchset adds support for '-cpu max' to Arm, along the lines > > of the existing support we have for x86 targets: > > > > * under KVM, -cpu max is the same as -cpu host > > * under TCG, -cpu max means "emulate with as many features as > >possible" > > Forgot to mention: -cpu max for qemu-system-aarch64 will > be a 64-bit cpu, and for qemu-system-arm it will be a 32-bit > cpu. (This differs from all the other TCG CPU types, which > behave the same for the 32-bit and 64-bit binaries. I think > it is the same way that x86 -cpu max works, though.) Are they going to be represented by two different QOM type names? (In the case of x86, all the CPU classes have different names on qemu-system-x86_64 and qemu-system-i386). -- Eduardo
Re: [Qemu-devel] [PATCH 13/17] iotests: Fix 067 for compat=0.10
On 11/22/2017 09:08 PM, Max Reitz wrote: > 067 works very well with compat=0.10 once you remove format-specific > information from the QMP output. > > Signed-off-by: Max Reitz Reviewed-by: John Snow
Re: [Qemu-devel] [PATCH for-2.12 0/4] qmp dirty bitmap API
This is going to be a long one. Maybe go get a cup of coffee.
On 12/07/2017 04:39 AM, Vladimir Sementsov-Ogievskiy wrote:
> 07.12.2017 03:38, John Snow wrote:
>> I'm sorry, I don't think I understand.
>>
>> "customers needs a possibility to create a backup of data changed since
>> some point in time."
>>
>> Is that not the existing case for a simple incremental backup? Granted,
>> the point in time was decided when we created the bitmap or when we made
>> the last backup, but it is "since some point in time."
>>
>> If you mean to say an arbitrary point in time after-the-fact, I don't
>> see how the API presented here helps enable that functionality.
>>
>> (by "arbitrary point in time after-the-fact I mean for example: Say a
>> user installs a malicious application in a VM on Thursday, but the
>> bitmap was created on Monday. The user wants to go back to Wednesday
>> evening, but we have no record of that point in time, so we cannot go
>> back to it.)
>>
>> Can you elaborate on what you're trying to accomplish so I make sure I'm
>> considering you carefully?
>
> Yes, point in time is when we create a dirty bitmap. But we want to
> maintain several points in time.
> For example it may be last 10 incremental backups.
> User wants an ability to create incremental backup which will contain
> changes from selected point in
> time to current moment. It is needed for example if backup was deleted
> (to save disk space) and now user
> wants to recreate it.
>
> In current scheme for incremental backup, after successful backup we
> actually lose previous point in time,
> and have only the last one.
Differential backup mode may help a little in this flexibility and costs
us basically nothing to implement:
We simply always re-merge the bitmaps after creating the backup. So you
have two options:
(1) Incremental: Replace the existing point-in-time with a new one
(2) Differential: Keep the existing point-in-time.
I suspect you are wanting something a lot more powerful than this, though.
>
> With ability to merge bitmap we can do the following:
>
> 1. create bitmap1
> 2. disable bitmap1, do external backup by bitmap1, create bitmap2
> 2.1 on backup fail: merge bitmap2 to bitmap1, enable bitmap1, delete
> bitmap2
> 2.2 on backup success: do nothing
Okay, so bitmap1 and bitmap2 cover periods of time that are disjoint;
where you have
time--->
[bitmap1][bitmap2-->
so you intend to accrue a number of bitmaps representing the last N
slices of time, with only the most recent bitmap being enabled.
Functionally you intend to permanently fork a bitmap every time a backup
operation succeeds; so on incremental backup:
(1) We succeed and the forked bitmap we already made gets saved as a new
disabled bitmap instead of being deleted.
(2) We fail, and we roll back exactly as we always have.
Here's an idea of what this API might look like without revealing
explicit merge/split primitives.
A new bitmap property that lets us set retention:
:: block-dirty-bitmap-set-retention bitmap=foo slices=10
Or something similar, where the default property for all bitmaps is zero
-- the current behavior: no copies retained.
By setting it to a non-zero positive integer, the incremental backup
mode will automatically save a disabled copy when possible.
"What happens if we exceed our retention?"
(A) We push the last one out automatically, or
(B) We fail the operation immediately.
A is more convenient, but potentially unsafe if the management tool or
user wasn't aware that was going to happen.
B is more annoying, but definitely more safe as it means we cannot lose
a bitmap accidentally.
I would argue for B with perhaps a force-cycle=true|false that defaults
to false to let management tools say "Yes, go ahead, remove the old one"
with additionally some return to let us know it happened:
{"return": {
"dropped-slices": [ {"bitmap0": 0}, ...]
}}
This would introduce some concept of bitmap slices into the mix as ID'd
children of a bitmap. I would propose that these slices are numbered and
monotonically increasing. "bitmap0" as an object starts with no slices,
but every incremental backup creates slice 0, slice 1, slice 2, and so
on. Even after we start deleting some, they stay ordered. These numbers
then stand in for points in time.
The counter can (must?) be reset and all slices forgotten when
performing a full backup while providing a bitmap argument.
"How can a user make use of the slices once they're made?"
Let's consider something like mode=partial in contrast to
mode=incremental, and an example where we have 6 prior slices:
0,1,2,3,4,5, (and, unnamed, the 'active' slice.)
mode=partial bitmap=foo slice=4
This would create a backup from slice 4 to the current time α. This
includes all clusters from 4, 5, and the active bitmap.
I don't think it is meaningful to define any end point that isn't the
current time, so I've omitted that as a possibility.
"Does a partial backup create a new point in time?"
If y
Re: [Qemu-devel] [PULL 08/13] target/arm: Pull Thumb insn word loads up to top level
On Thu, Oct 12, 2017 at 17:03:31 +0100, Peter Maydell wrote: > Refactor the Thumb decode to do the loads of the instruction words at > the top level rather than only loading the second half of a 32-bit > Thumb insn in the middle of the decode. > > This is simple apart from the awkward case of Thumb1, where the > BL/BLX prefix and suffix instructions live in what in Thumb2 is the > 32-bit insn space. To handle these we decode enough to identify > whether we're looking at a prefix/suffix that we handle as a 16 bit > insn, or a prefix that we're going to merge with the following suffix > to consider as a 32 bit insn. The translation of the 16 bit cases > then moves from disas_thumb2_insn() to disas_thumb_insn(). > > The refactoring has the benefit that we don't need to pass the > CPUARMState* down into the decoder code any more, but the major > reason for doing this is that some Thumb instructions must be always > unconditional regardless of the IT state bits, so we need to know the > whole insn before we emit the "skip this insn if the IT bits and cond > state tell us to" code. (The always unconditional insns are BKPT, > HLT and SG; the last of these is 32 bits.) > > Signed-off-by: Peter Maydell > Reviewed-by: Richard Henderson > Message-id: 1507556919-24992-7-git-send-email-peter.mayd...@linaro.org This commit breaks the debian-arm boot test (see [1]), boot dies at: > random: systemd urandom read with 4 bits of entropy available > systemd[1]: Caught , core dump failed. > systemd[1]: Freezing execution. Sorry I noticed this so late (-rc4), for a while I thought my development was causing this and didn't pay much attention to it. However, just realised the problem is present on master. Bisect log below. Thanks, Emilio [1] https://lists.gnu.org/archive/html/qemu-devel/2017-06/msg04085.html $ git bisect log git bisect start # bad: [2babfe0c9241c239272a03fec785165a50e8288c] Update version for v2.11.0-rc4 release git bisect bad 2babfe0c9241c239272a03fec785165a50e8288c # good: [1ab5eb4efb91a3d4569b0df6e824cc08ab4bd8ec] Update version for v2.10.0 release git bisect good 1ab5eb4efb91a3d4569b0df6e824cc08ab4bd8ec # good: [62a2554ec2630896d1299e1a282a64c7f3b00da0] 390x/css: introduce maximum data address checking git bisect good 62a2554ec2630896d1299e1a282a64c7f3b00da0 # bad: [f51f315a676ec913a55ac27be4ef857f9f7ddc5c] translate-all: use qemu_protect_rwx/none helpers git bisect bad f51f315a676ec913a55ac27be4ef857f9f7ddc5c # bad: [9f99c85c4a364f8de8134eb53b0cc1b84ded4b3f] Merge remote-tracking branch 'remotes/gkurz/tags/for-upstream' into staging git bisect bad 9f99c85c4a364f8de8134eb53b0cc1b84ded4b3f # good: [3637cf58f9441ad277fd70299a29d0e39b32c96c] util: move qemu_real_host_page_size/mask to osdep.h git bisect good 3637cf58f9441ad277fd70299a29d0e39b32c96c # bad: [b81b948ecc8659d78066f374c787ed12379d21dd] virtio/pci/migration: Convert to VMState git bisect bad b81b948ecc8659d78066f374c787ed12379d21dd # good: [43851b5bd48d952561610d0d6d6c314c97eff543] iotests: Set up Python logging git bisect good 43851b5bd48d952561610d0d6d6c314c97eff543 # bad: [76eff04d166b8fe747adbe82de8b7e060e668ff9] target/arm: Implement SG instruction corner cases git bisect bad 76eff04d166b8fe747adbe82de8b7e060e668ff9 # good: [b9f587d62cebed427206539750ebf59bde4df422] target/arm: Add M profile secure MMU index values to get_a32_user_mem_index() git bisect good b9f587d62cebed427206539750ebf59bde4df422 # good: [6b8acf256df09c8a8dd7dcaa79b06eaff4ad63f7] target-arm: Don't check for "Thumb2 or M profile" for not-Thumb1 git bisect good 6b8acf256df09c8a8dd7dcaa79b06eaff4ad63f7 # bad: [5b8d7289e9e92a0d7bcecb93cd189e245fef10cd] target-arm: Simplify insn_crosses_page() git bisect bad 5b8d7289e9e92a0d7bcecb93cd189e245fef10cd # bad: [296e5a0a6c393553079a641c50521ae33ff89324] target/arm: Pull Thumb insn word loads up to top level git bisect bad 296e5a0a6c393553079a641c50521ae33ff89324 # first bad commit: [296e5a0a6c393553079a641c50521ae33ff89324] target/arm: Pull Thumb insn word loads up to top level
Re: [Qemu-devel] [PATCH v5 01/23] memattrs: add debug attribute
On 12/8/17 3:55 AM, Peter Maydell wrote:
>> If you give me example on how
>> to trigger this type of request with debug=1 then I can look into the code
>> and see what we can do when memory encryption is enabled. The things like
>> read-clears-bits semantics will be tricky.
> The question was really whether we want to make this a general
> indicator of "this operation was triggered by a debugger" and
> expand that to mean "don't do things that mess with the state
> of the simulation unexpectedly", or if this is really a very
> encrypted-memory specific thing.
It can be used as a generic indicator that operation was triggered by a
debugger. There is not anything encryption specific. Having said that,
in the current patch series I have been only focused on making it work
from the gdbstub and HMP point of view. The debug=1 from gdbstub and HMP
is tested on both encrypted and non-encrypted guest. If we decide to
extend to support other callers (device model etc) then we may need to
update memory load/store functions defined in memory_ldst_inc.c to work
with debug=1.
> By the way, I don't think this:
>
>> /* Access the guest memory for debug purposes */
>> #define MEMTXATTRS_DEBUG ((MemTxAttrs) { .debug = 1 })
> is a great idea. Callers that care about the transaction
> attributes should just specify them properly. MEMTXATTRS_UNSPECIFIED
> is a fallback for the large set of places that don't care at all.
OK, I will drop the macro and update the patches in the series to set
MemTxAttr.debug = 1 when we do debugger request. thanks
--Brijesh
Re: [Qemu-devel] [PATCH] xen/pt: Set is_express to avoid out-of-bounds write
On Sat, 28 Oct 2017, Simon Gaiser wrote: > The passed-through device might be an express device. In this case the > old code allocated a too small emulated config space in > pci_config_alloc() since pci_config_size() returned the size for a > non-express device. This leads to an out-of-bound write in > xen_pt_config_reg_init(), which sometimes results in crashes. So set > is_express as already done for KVM in vfio-pci. > > Shortened ASan report: > > ==17512==ERROR: AddressSanitizer: heap-buffer-overflow on address > 0x61141648 at pc 0x55e0fdac51ff bp 0x7ffe4af07410 sp 0x7ffe4af07408 > WRITE of size 2 at 0x61141648 thread T0 > #0 0x55e0fdac51fe in memcpy > /usr/include/x86_64-linux-gnu/bits/string3.h:53 > #1 0x55e0fdac51fe in stw_he_p include/qemu/bswap.h:330 > #2 0x55e0fdac51fe in stw_le_p include/qemu/bswap.h:379 > #3 0x55e0fdac51fe in pci_set_word include/hw/pci/pci.h:490 > #4 0x55e0fdac51fe in xen_pt_config_reg_init > hw/xen/xen_pt_config_init.c:1991 > #5 0x55e0fdac51fe in xen_pt_config_init hw/xen/xen_pt_config_init.c:2067 > #6 0x55e0fdabcf4d in xen_pt_realize hw/xen/xen_pt.c:830 > #7 0x55e0fdf59666 in pci_qdev_realize hw/pci/pci.c:2034 > #8 0x55e0fdda7d3d in device_set_realized hw/core/qdev.c:914 > [...] > > 0x61141648 is located 8 bytes to the right of 256-byte region > [0x61141540,0x61141640) > allocated by thread T0 here: > #0 0x7ff596a94bb8 in __interceptor_calloc > (/usr/lib/x86_64-linux-gnu/libasan.so.4+0xd9bb8) > #1 0x7ff57da66580 in g_malloc0 > (/lib/x86_64-linux-gnu/libglib-2.0.so.0+0x50580) > #2 0x55e0fdda7d3d in device_set_realized hw/core/qdev.c:914 > [...] > > Signed-off-by: Simon Gaiser Acked-by: Stefano Stabellini > --- > > I found this by debugging crashes and I'm not familiar with this code, > so I'm not sure if this has no unintended side effects. > > hw/xen/xen_pt.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/hw/xen/xen_pt.c b/hw/xen/xen_pt.c > index b6d71bb52a..90ffd45e7d 100644 > --- a/hw/xen/xen_pt.c > +++ b/hw/xen/xen_pt.c > @@ -946,6 +946,7 @@ static void xen_pci_passthrough_class_init(ObjectClass > *klass, void *data) > k->exit = xen_pt_unregister_device; > k->config_read = xen_pt_pci_read_config; > k->config_write = xen_pt_pci_write_config; > +k->is_express = 1; /* We might be */ > set_bit(DEVICE_CATEGORY_MISC, dc->categories); > dc->desc = "Assign an host PCI device with Xen"; > dc->props = xen_pci_passthrough_properties; > -- > 2.15.0.rc1 > >
[Qemu-devel] [Bug 1736042] Re: qemu-system-x86_64 does not boot image reliably
This looks not a QEMU bug to me. You may drop "-curses" first, and run again. Once get inside, change grub file(/etc/default/grub) by uncomment GRUB_TERMINAL=console. It should work then. If still not, then blacklist vga16fb and add "fbcon=map:99 text" in grub command line. Remember to run update-grub after change configure file. -- You received this bug notification because you are a member of qemu- devel-ml, which is subscribed to QEMU. https://bugs.launchpad.net/bugs/1736042 Title: qemu-system-x86_64 does not boot image reliably Status in QEMU: New Bug description: Booting image as root user with following command works randomly. ./qemu-system-x86_64 -enable-kvm -curses -smp cpus=4 -m 8192 /root/ructfe2917_g.qcow2 Most of the time it ends up on "800x600 Graphic mode"(been stuck there even for 4 hours before killed), but 1 out of ~20 it boots image correctly(and instantly). This is visible in v2.5.0 build from sources, v2.5.0 from Ubuntu Xenial and v2.1.2 from Debian Jessie. The image in question was converted from vmdk using: qemu-img convert -O qcow2 file.vmdk file.qcow2 The image contains Ubuntu with grub. I can provide debug logs, but will need guidance how to enable them(and what logs are necessary). As a side note, it seems that booting is more certain after connecting(or mounting) partition using qemu-nbd/mount. To manage notifications about this bug go to: https://bugs.launchpad.net/qemu/+bug/1736042/+subscriptions
Re: [Qemu-devel] [PATCH 00/12] Refactor get_phys_addr() not to return FSR values
On Tue, 5 Dec 2017, Peter Maydell wrote: > Currently get_phys_addr() and its various subfunctions return a > hard-coded fault status register value for translation failures. This > is awkward because FSR values these days may be either long-descriptor > format or short-descriptor format. Worse, the right FSR type to use > doesn't depend only on the translation table being walked -- some > cases, like fault info reported to AArch32 EL2 for some kinds of ATS > operation, must be in long-descriptor format even if the translation > table being walked was short format. We can't get those cases right > with our current approach. (We put in a bodge fix so we could at > least run Xen, in commit 50cd71b0d347c7.) > > This series does a refactoring of get_phys_addr() so that instead > of returning FSR values it puts information in the existing > ARMMMUFaultInfo structure that is sufficient to construct an > FSR value, and the callers then do that using the right condition > for whatever they're doing. I've included Edgar's patch from back > in June that fixes the handling of ATS instructions, which is most > of the point of the refactoring. > > Rather than doing it all in one massive unreviewable patch, the > series is structured as: > * patch 1: add fields to ARMMMUFaultInfo, and the utility functions >that return a long or short format FSR given a fault info struct > * patch 2: stop passing an fsr argument to arm_ld*_ptw(), since >nothing actually looks at the result. (This breaks a cycle >arm_ldq_ptw()->S1_ptw_translate()->get_phys_addr_lpae()->arm_ldq_ptw() >which would otherwise make a stepwise refactoring awkward.) > * patches 3 to 8: for each of the different subfunctions of >get_phys_addr(), make them fill in the fault info fields instead >of an fsr value. Temporarily we add calls to arm_fi_to_sfsc() >and arm_fi_to_lfsc() at the callsites in get_phys_addr(), since >the callers of get_phys_addr() still need the fsr values. These >will go away again in a later patch. > * patches 9 and 10: change the callers of get_phys_addr() to use >the info in the fault info struct and ignore the fsr value. > * patch 11: remove the now unused fsr argument (and the temporary >calls to arm_fi_to_sfsc()/arm_fi_to_lfsc()) > * patch 12: Edgar's patch for ATS PAR format > > Arguably it would be good to push the "create the FSR value" > logic further, into the point where the exception is taken. > (This would let us correct the oddity where for M profile we > get an A/R profile FSR value in arm_v7m_cpu_do_interrupt() > which we then have to convert into the appropriate M profile > fault status bits.) However, migration backcompat makes this > tricky, because at the moment we migrate env->exception.fsr, > and so we'd need to have code just to handle inbound migrations > from old QEMU with an FSR and reconstitute a fault info struct. > So I've stopped here, at least for now. > > This whole series sits on top of my v8M TT patchset (which also > had to touch some of the get_phys_addr() code). I've pushed it > to this git branch if that's more convenient: > > https://git.linaro.org/people/peter.maydell/qemu-arm.git fsr-in-faultinfo > > I have tested a bit (including a Linux KVM EL2 setup), but more > testing would definitely be useful. Stefano: in particular it would > be good to check this hasn't broken Xen again :-) Still works :-) Tested-by: Stefano Stabellini > Based-on: 1512153879-5291-1-git-send-email-peter.mayd...@linaro.org > ([PATCH 0/7] armv8m: Implement TT, and other bugfixes) > > thanks > -- PMM > > > Edgar E. Iglesias (1): > target/arm: Extend PAR format determination > > Peter Maydell (11): > target/arm: Provide fault type enum and FSR conversion functions > target/arm: Remove fsr argument from arm_ld*_ptw() > target/arm: Convert get_phys_addr_v5() to not return FSC values > target/arm: Convert get_phys_addr_v6() to not return FSC values > target/arm: Convert get_phys_addr_lpae() to not return FSC values > target/arm: Convert get_phys_addr_pmsav5() to not return FSC values > target/arm: Convert get_phys_addr_pmsav7() to not return FSC values > target/arm: Convert get_phys_addr_pmsav8() to not return FSC values > target/arm: Use ARMMMUFaultInfo in deliver_fault() > target/arm: Ignore fsr from get_phys_addr() in do_ats_write() > target/arm: Remove fsr argument from get_phys_addr() and > arm_tlb_fill() > > target/arm/internals.h | 187 +- > target/arm/helper.c| 238 > +++-- > target/arm/op_helper.c | 82 + > 3 files changed, 342 insertions(+), 165 deletions(-) > > -- > 2.7.4 > >
Re: [Qemu-devel] [PATCH v6 21/26] tcg: Add generic vector ops for multiplication
On 12/05/2017 03:33 AM, Kirill Batuzov wrote:
> On Tue, 21 Nov 2017, Richard Henderson wrote:
>
>> Signed-off-by: Richard Henderson
>
>> +void tcg_gen_mul_vec(unsigned vece, TCGv_vec r, TCGv_vec a, TCGv_vec b)
>> +{
>> +TCGTemp *rt = tcgv_vec_temp(r);
>> +TCGTemp *at = tcgv_vec_temp(a);
>> +TCGTemp *bt = tcgv_vec_temp(b);
>> +TCGArg ri = temp_arg(rt);
>> +TCGArg ai = temp_arg(at);
>> +TCGArg bi = temp_arg(bt);
>> +TCGType type = rt->base_type;
>> +int can;
>> +
>> +tcg_debug_assert(at->base_type == type);
>> +tcg_debug_assert(bt->base_type == type);
>> +can = tcg_can_emit_vec_op(INDEX_op_cmp_vec, type, vece);
>
> Should be INDEX_op_mul_vec in the line above.
Fixed, thanks.
r~
Re: [Qemu-devel] [PATCH v6 05/26] tcg: Add generic vector expanders
On 12/06/2017 02:21 AM, Kirill Batuzov wrote: > IN: > 0x004005e8: d2824682 movz x2, #0x1234 > 0x004005ec: 4e010c41 dup v1.16b, w2 > > OP: > > 004005e8 > movi_i64 x2,$0x1234 > > 004005ec > st_i32 x2,env,$0x8b0 > st_i32 x2,env,$0x8b4 > st_i32 x2,env,$0x8b8 > st_i32 x2,env,$0x8bc Fixed. 00400078 movi_i64 x2,$0x1234 sync: 0 dead: 0 0040007c movi_i32 tmp0,$0x34343434 st_i32 tmp0,env,$0x8b0 st_i32 tmp0,env,$0x8b4 st_i32 tmp0,env,$0x8b8 st_i32 tmp0,env,$0x8bc dead: 0 Thanks, r~
Re: [Qemu-devel] [PATCH qemu] fdc: Exit if ISA controller does not support DMA
On 11/21/2017 09:48 PM, Alexey Kardashevskiy wrote:
> On 07/11/17 11:58, John Snow wrote:
>>
>>
>> On 10/26/2017 02:46 AM, Alexey Kardashevskiy wrote:
>>> A "powernv" machine type defines an ISA bus but it does not add any DMA
>>> controller to it so it is possible to hit assert(fdctrl->dma) by
>>> adding "-machine powernv -device isa-fdc".
>>>
>>> This replaces assert() with an error message.
>>>
>>> Signed-off-by: Alexey Kardashevskiy
>>> ---
>>>
>>> Is it a must for ISA to have DMA controllers?
>>>
>>>
>>> ---
>>> hw/block/fdc.c | 5 -
>>> 1 file changed, 4 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/hw/block/fdc.c b/hw/block/fdc.c
>>> index 67f78ac702..ed8b367572 100644
>>> --- a/hw/block/fdc.c
>>> +++ b/hw/block/fdc.c
>>> @@ -2700,7 +2700,10 @@ static void isabus_fdc_realize(DeviceState *dev,
>>> Error **errp)
>>> fdctrl->dma_chann = isa->dma;
>>> if (fdctrl->dma_chann != -1) {
>>> fdctrl->dma = isa_get_dma(isa_bus_from_device(isadev), isa->dma);
>>> -assert(fdctrl->dma);
>>> +if (!fdctrl->dma) {
>>> +error_setg(errp, "ISA controller does not support DMA,
>>> exiting");
>>> +return;
>>> +}
>>> }
>>>
>>> qdev_set_legacy_instance_id(dev, isa->iobase, 2);
>>>
>>
>> I've been MIA for a little while, so I'm out of the loop -- but I am not
>> sure this is entirely the right way to fix this problem. I think it is
>> more the case that certain boards should not be able to ask for certain
>> types of devices, and we should prohibit e.g. powernv from being able to
>> ask for an ISA floppy disk controller.
>>
>> (It doesn't seem to have an ISA DMA controller by default, but I have no
>> idea if that means it can't EVER have one...)
>>
>> Papering over this by making it a soft error when we fail to execute
>> isa_get_dma and then assuming in retrospect it's because the machine
>> type we're on cannot have an ISA DMA controller seems a little
>> wrong-headed. It also leaves side-effects from isa_register_portio_list
>> and isa_init_irq, so we can't just bail here -- it's only marginally
>> better than the assert() it's doing.
>>
>> That said, I am not really sure what the right thing to do is ... I
>> suspect the "right thing" is to express the dependency that isa-fdc
>> requires an ISA DMA controller -- and maybe that check happens here when
>> isa_get_dma fails and we have to unwind the realize function, but we
>> need to do it gracefully.
>>
>> Give me a day to think about it, but I do want to make sure this is in
>> the next release.
>
>
> The day has passed, any news? :)
>
>
*cough* It turns out that understanding the intricacies of FDC and ISA
is nobody's favorite thing to do.
OK, so ehabkost pointed me to this:
https://www.mail-archive.com/qemu-devel@nongnu.org/msg496460.html
Where we declare that DMA devices generally can't be created by the user
for the inverse of the reason we're seeing here: these devices need to
be created precisely once: not zero times, not twice, exactly once.
So we made the ISA DMA devices themselves not user-creatable, so you are
indeed correct here that the absence of fdctrl->dma does more or less
mean that the current configuration "doesn't support DMA." ... but maybe
this won't always be true, and maybe some devices (TYPE_I82374?) are
user creatable, so let's make a "softer" error message:
"No ISA DMA device present, can't create ISA FDC device."
Then, on the other end, we need to unwind realize() gracefully, maybe we
can just shuffle up isa_get_dma() earlier so we don't have to unwind
anything if it comes back empty.
Then I'll take the patch, because fixing this more properly I think will
take more time or effort than I have to spend on the FDC device.
Thanks to Eduardo for looking this over with me.
--js
As a post-script, ehabkost dug this up too:
https://www.mail-archive.com/qemu-devel@nongnu.org/msg348764.html
It looks like Herve was working on decoupling floppies from i8257, but
perhaps didn't get all the way through -- I'm not actually clear on what
work remains to be done here, maybe he can chime in if he's still
interested in the project?
Re: [Qemu-devel] [PATCH 1/2] tpm_spapr: Support TPM for ppc64 using CRQ based interface
On 12/08/2017 01:17 PM, Cédric Le Goater wrote: On 12/06/2017 12:53 PM, Stefan Berger wrote: On 12/06/2017 02:26 AM, Cédric Le Goater wrote: Hello Stefan, Some comments below. How do we populate the device tree ? Patched SLOF. Ah. Isn't that a problem ? I thought QEMU was in charge of populating the device tree. I haven't seen this for ppc64 but it's true for ACPI on x86. You should send the full patchset rebased on QEMU's head or on David's 2.12 branch else this patch won't apply or compile. I am maintaining a tpm-next branch for 2.12 that includes the patches that this builds on top of. Stefan
Re: [Qemu-devel] [RFC] vhost: check if ring mapping is still valid when building memmap
* Igor Mammedov (imamm...@redhat.com) wrote:
> vhost_verify_ring_mappings() were used to verify that
> rings are still accessible and related memory hasn't
> been moved after flatview is updated.
I think I've rolled the equivalent into my v2 I've just
posted; please have a look.
Dave
> It were doing checks by mapping ring's GPA+len and
> checking that HVA hasn't changed with new memory map.
> To avoid maybe expensive mapping call, we were
> identifying address range that changed and were doing
> mapping only if ring were in changed range.
>
> However it's no neccessary to perform ringi's GPA
> mapping as we already have it's current HVA and all
> we need is to verify that ring's GPA translates to
> the same HVA in updated flatview.
>
> That could be done during time when we are building
> vhost memmap where vhost_update_mem_cb() already maps
> every RAM MemoryRegionSection to get section HVA. This
> fact can be used to check that ring belongs to the same
> MemoryRegion in the same place, like this:
>
> hva_ring_offset = GPA(ring) - GPA(MemoryRegionSection)
> ring_hva == HVA(MemoryRegionSection) + hva_ring_offset
>
> Doing this would allow us to drop ~100LOC of code which
> figures out changed memory range and avoid calling not
> needed reverse vhost_memory_map(is_write=true) which is
> overkill for the task at hand.
>
> Signed-off-by: Igor Mammedov
> ---
> PS:
>should be applied on top of David's series
>
> ---
> include/hw/virtio/vhost.h | 2 -
> hw/virtio/vhost.c | 195
> ++
> 2 files changed, 57 insertions(+), 140 deletions(-)
>
> diff --git a/include/hw/virtio/vhost.h b/include/hw/virtio/vhost.h
> index 467dc77..fc4af5c 100644
> --- a/include/hw/virtio/vhost.h
> +++ b/include/hw/virtio/vhost.h
> @@ -68,8 +68,6 @@ struct vhost_dev {
> uint64_t log_size;
> Error *migration_blocker;
> bool memory_changed;
> -hwaddr mem_changed_start_addr;
> -hwaddr mem_changed_end_addr;
> const VhostOps *vhost_ops;
> void *opaque;
> struct vhost_log *log;
> diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c
> index 5b9a7d7..026bac3 100644
> --- a/hw/virtio/vhost.c
> +++ b/hw/virtio/vhost.c
> @@ -296,35 +296,43 @@ static void vhost_memory_unmap(struct vhost_dev *dev,
> void *buffer,
> }
> }
>
> -static int vhost_verify_ring_part_mapping(struct vhost_dev *dev,
> - void *part,
> - uint64_t part_addr,
> - uint64_t part_size,
> - uint64_t start_addr,
> - uint64_t size)
> +static int vhost_verify_ring_part_mapping(void *ring_hva,
> + uint64_t ring_gpa,
> + uint64_t ring_size,
> + void *reg_hva,
> + uint64_t reg_gpa,
> + uint64_t reg_size)
> {
> -hwaddr l;
> -void *p;
> -int r = 0;
> +uint64_t hva_ring_offset;
> +uint64_t ring_last = range_get_last(ring_gpa, ring_size);
> +uint64_t reg_last = range_get_last(reg_gpa, reg_size);
>
> -if (!ranges_overlap(start_addr, size, part_addr, part_size)) {
> +/* start check from the end so that the rest of checks
> + * are done when whole ring is in merged sections range
> + */
> +if (ring_last < reg_last || ring_gpa > reg_last) {
> return 0;
> }
> -l = part_size;
> -p = vhost_memory_map(dev, part_addr, &l, 1);
> -if (!p || l != part_size) {
> -r = -ENOMEM;
> +
> +/* check that whole ring's is mapped */
> +if (range_get_last(ring_gpa, ring_size) >
> +range_get_last(reg_gpa, reg_size)) {
> +return -EBUSY;
> }
> -if (p != part) {
> -r = -EBUSY;
> +
> +/* check that ring's MemoryRegion wasn't replaced */
> +hva_ring_offset = ring_gpa - reg_gpa;
> +if (ring_hva != reg_hva + hva_ring_offset) {
> +return -ENOMEM;
> }
> -vhost_memory_unmap(dev, p, l, 0, 0);
> -return r;
> +
> +return 0;
> }
>
> static int vhost_verify_ring_mappings(struct vhost_dev *dev,
> - uint64_t start_addr,
> - uint64_t size)
> + void *reg_hva,
> + uint64_t reg_gpa,
> + uint64_t reg_size)
> {
> int i, j;
> int r = 0;
> @@ -338,23 +346,26 @@ static int vhost_verify_ring_mappings(struct vhost_dev
> *dev,
> struct vhost_virtqueue *vq = dev->vqs + i;
>
> j = 0;
> -r = vhost_verify_ring_part_mapping(dev, vq->desc, vq->desc_phys,
> - vq->desc_size, start_addr, size);
> -if (!r) {
>
[Qemu-devel] [RFC v2 8/8] vhost: Move mem_sections maintenance into commit/update routines
From: "Dr. David Alan Gilbert"
Mvoe the maintenance of mem_sections into the vhost_update_mem routines,
this removes the need for the vhost_region_add/del callbacks.
Suggested-by: Igor Mammedov
(and mostly written by Igor!)
Signed-off-by: Dr. David Alan Gilbert
---
hw/virtio/vhost.c | 58 +++
1 file changed, 16 insertions(+), 42 deletions(-)
diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c
index 2cfb13272f..79927ab93d 100644
--- a/hw/virtio/vhost.c
+++ b/hw/virtio/vhost.c
@@ -408,6 +408,13 @@ static int vhost_update_mem_cb(MemoryRegionSection *mrs,
void *opaque)
if (!vhost_section(mrs)) {
return 0;
}
+++vtmp->dev->n_mem_sections;
+vtmp->dev->mem_sections = g_renew(MemoryRegionSection,
+ vtmp->dev->mem_sections,
+ vtmp->dev->n_mem_sections);
+vtmp->dev->mem_sections[vtmp->dev->n_mem_sections - 1] = *mrs;
+memory_region_ref(mrs->mr);
+
mrs_size = int128_get64(mrs->size);
mrs_gpa = mrs->offset_within_address_space;
mrs_host = (uintptr_t)memory_region_get_ram_ptr(mrs->mr) +
@@ -461,6 +468,7 @@ static int vhost_update_mem_cb(MemoryRegionSection *mrs,
void *opaque)
static int vhost_update_mem(struct vhost_dev *dev, bool *changed)
{
int res;
+unsigned i;
struct vhost_update_mem_tmp vtmp;
size_t mem_size;
vtmp.regions = 0;
@@ -469,6 +477,14 @@ static int vhost_update_mem(struct vhost_dev *dev, bool
*changed)
trace_vhost_update_mem();
*changed = false;
+/* Clear out the section list, it'll get rebuilt */
+for (i = 0; i < dev->n_mem_sections; i++) {
+memory_region_unref(dev->mem_sections[i].mr);
+}
+g_free(dev->mem_sections);
+dev->mem_sections = NULL;
+dev->n_mem_sections = 0;
+
res = address_space_iterate(&address_space_memory,
vhost_update_mem_cb, &vtmp);
if (res) {
@@ -553,46 +569,6 @@ static void vhost_commit(MemoryListener *listener)
}
}
-static void vhost_region_add(MemoryListener *listener,
- MemoryRegionSection *section)
-{
-struct vhost_dev *dev = container_of(listener, struct vhost_dev,
- memory_listener);
-
-if (!vhost_section(section)) {
-return;
-}
-
-++dev->n_mem_sections;
-dev->mem_sections = g_renew(MemoryRegionSection, dev->mem_sections,
-dev->n_mem_sections);
-dev->mem_sections[dev->n_mem_sections - 1] = *section;
-memory_region_ref(section->mr);
-}
-
-static void vhost_region_del(MemoryListener *listener,
- MemoryRegionSection *section)
-{
-struct vhost_dev *dev = container_of(listener, struct vhost_dev,
- memory_listener);
-int i;
-
-if (!vhost_section(section)) {
-return;
-}
-
-memory_region_unref(section->mr);
-for (i = 0; i < dev->n_mem_sections; ++i) {
-if (dev->mem_sections[i].offset_within_address_space
-== section->offset_within_address_space) {
---dev->n_mem_sections;
-memmove(&dev->mem_sections[i], &dev->mem_sections[i+1],
-(dev->n_mem_sections - i) * sizeof(*dev->mem_sections));
-break;
-}
-}
-}
-
static void vhost_iommu_unmap_notify(IOMMUNotifier *n, IOMMUTLBEntry *iotlb)
{
struct vhost_iommu *iommu = container_of(n, struct vhost_iommu, n);
@@ -1161,8 +1137,6 @@ int vhost_dev_init(struct vhost_dev *hdev, void *opaque,
hdev->memory_listener = (MemoryListener) {
.commit = vhost_commit,
-.region_add = vhost_region_add,
-.region_del = vhost_region_del,
.region_nop = vhost_region_nop,
.log_start = vhost_log_start,
.log_stop = vhost_log_stop,
--
2.14.3
[Qemu-devel] [RFC v2 6/8] vhost: Compare and copy updated region data into device state
From: "Dr. David Alan Gilbert"
Compare the temporary region data with the original, and if it's
different update the original in the device state.
Signed-off-by: Dr. David Alan Gilbert
---
hw/virtio/trace-events | 2 ++
hw/virtio/vhost.c | 19 +--
2 files changed, 19 insertions(+), 2 deletions(-)
diff --git a/hw/virtio/trace-events b/hw/virtio/trace-events
index 92fadec192..fac89aaba5 100644
--- a/hw/virtio/trace-events
+++ b/hw/virtio/trace-events
@@ -2,6 +2,8 @@
# hw/virtio/vhost.c
vhost_section(const char *name, int r) "%s:%d"
+vhost_update_mem(void) ""
+vhost_update_mem_changed(void) ""
vhost_update_mem_cb(const char *name, uint64_t gpa, uint64_t size, uint64_t
host) "%s: 0x%"PRIx64"+0x%"PRIx64" @ 0x%"PRIx64
vhost_update_mem_cb_abut(const char *name, uint64_t new_size) "%s: 0x%"PRIx64
diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c
index 9d39271bfc..7ac64ad056 100644
--- a/hw/virtio/vhost.c
+++ b/hw/virtio/vhost.c
@@ -707,10 +707,12 @@ static int vhost_update_mem(struct vhost_dev *dev, bool
*changed)
{
int res;
struct vhost_update_mem_tmp vtmp;
+size_t mem_size;
vtmp.regions = 0;
vtmp.nregions = 0;
vtmp.dev = dev;
+trace_vhost_update_mem();
*changed = false;
res = address_space_iterate(&address_space_memory,
vhost_update_mem_cb, &vtmp);
@@ -718,8 +720,21 @@ static int vhost_update_mem(struct vhost_dev *dev, bool
*changed)
goto out;
}
-/* TODO */
-*changed = dev->mem_changed_start_addr > dev->mem_changed_end_addr;
+mem_size = offsetof(struct vhost_memory, regions) +
+ (vtmp.nregions + 1) * sizeof dev->mem->regions[0];
+
+if (vtmp.nregions != dev->mem->nregions ||
+ memcmp(vtmp.regions, dev->mem->regions, mem_size)) {
+*changed = true;
+/* Update the main regions list from our tmp */
+dev->mem = g_realloc(dev->mem, mem_size);
+dev->mem->nregions = vtmp.nregions;
+memcpy(dev->mem->regions, vtmp.regions,
+ vtmp.nregions * sizeof dev->mem->regions[0]);
+used_memslots = vtmp.nregions;
+trace_vhost_update_mem_changed();
+}
+
out:
g_free(vtmp.regions);
return res;
--
2.14.3
[Qemu-devel] [RFC v2 7/8] vhost: Remove old vhost_set_memory etc
From: "Dr. David Alan Gilbert"
Remove the old update mechanism, vhost_set_memory, and the functions
it uses and the memory_changed flags we no longer use.
Signed-off-by: Dr. David Alan Gilbert
---
hw/virtio/vhost.c | 254 --
include/hw/virtio/vhost.h | 3 -
2 files changed, 257 deletions(-)
diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c
index 7ac64ad056..2cfb13272f 100644
--- a/hw/virtio/vhost.c
+++ b/hw/virtio/vhost.c
@@ -156,160 +156,6 @@ static void vhost_log_sync_range(struct vhost_dev *dev,
}
}
-/* Assign/unassign. Keep an unsorted array of non-overlapping
- * memory regions in dev->mem. */
-static void vhost_dev_unassign_memory(struct vhost_dev *dev,
- uint64_t start_addr,
- uint64_t size)
-{
-int from, to, n = dev->mem->nregions;
-/* Track overlapping/split regions for sanity checking. */
-int overlap_start = 0, overlap_end = 0, overlap_middle = 0, split = 0;
-
-for (from = 0, to = 0; from < n; ++from, ++to) {
-struct vhost_memory_region *reg = dev->mem->regions + to;
-uint64_t reglast;
-uint64_t memlast;
-uint64_t change;
-
-/* clone old region */
-if (to != from) {
-memcpy(reg, dev->mem->regions + from, sizeof *reg);
-}
-
-/* No overlap is simple */
-if (!ranges_overlap(reg->guest_phys_addr, reg->memory_size,
-start_addr, size)) {
-continue;
-}
-
-/* Split only happens if supplied region
- * is in the middle of an existing one. Thus it can not
- * overlap with any other existing region. */
-assert(!split);
-
-reglast = range_get_last(reg->guest_phys_addr, reg->memory_size);
-memlast = range_get_last(start_addr, size);
-
-/* Remove whole region */
-if (start_addr <= reg->guest_phys_addr && memlast >= reglast) {
---dev->mem->nregions;
---to;
-++overlap_middle;
-continue;
-}
-
-/* Shrink region */
-if (memlast >= reglast) {
-reg->memory_size = start_addr - reg->guest_phys_addr;
-assert(reg->memory_size);
-assert(!overlap_end);
-++overlap_end;
-continue;
-}
-
-/* Shift region */
-if (start_addr <= reg->guest_phys_addr) {
-change = memlast + 1 - reg->guest_phys_addr;
-reg->memory_size -= change;
-reg->guest_phys_addr += change;
-reg->userspace_addr += change;
-assert(reg->memory_size);
-assert(!overlap_start);
-++overlap_start;
-continue;
-}
-
-/* This only happens if supplied region
- * is in the middle of an existing one. Thus it can not
- * overlap with any other existing region. */
-assert(!overlap_start);
-assert(!overlap_end);
-assert(!overlap_middle);
-/* Split region: shrink first part, shift second part. */
-memcpy(dev->mem->regions + n, reg, sizeof *reg);
-reg->memory_size = start_addr - reg->guest_phys_addr;
-assert(reg->memory_size);
-change = memlast + 1 - reg->guest_phys_addr;
-reg = dev->mem->regions + n;
-reg->memory_size -= change;
-assert(reg->memory_size);
-reg->guest_phys_addr += change;
-reg->userspace_addr += change;
-/* Never add more than 1 region */
-assert(dev->mem->nregions == n);
-++dev->mem->nregions;
-++split;
-}
-}
-
-/* Called after unassign, so no regions overlap the given range. */
-static void vhost_dev_assign_memory(struct vhost_dev *dev,
-uint64_t start_addr,
-uint64_t size,
-uint64_t uaddr)
-{
-int from, to;
-struct vhost_memory_region *merged = NULL;
-for (from = 0, to = 0; from < dev->mem->nregions; ++from, ++to) {
-struct vhost_memory_region *reg = dev->mem->regions + to;
-uint64_t prlast, urlast;
-uint64_t pmlast, umlast;
-uint64_t s, e, u;
-
-/* clone old region */
-if (to != from) {
-memcpy(reg, dev->mem->regions + from, sizeof *reg);
-}
-prlast = range_get_last(reg->guest_phys_addr, reg->memory_size);
-pmlast = range_get_last(start_addr, size);
-urlast = range_get_last(reg->userspace_addr, reg->memory_size);
-umlast = range_get_last(uaddr, size);
-
-/* check for overlapping regions: should never happen. */
-assert(prlast < start_addr || pmlast < reg->guest_phys_addr);
-/* Not an adjacent or overlapping region - do not merge. */
-if ((prlast + 1 != start_addr || urlast + 1 != uaddr) &&
-(pmlast + 1 != reg->guest_phys_addr ||
-
[Qemu-devel] [RFC v2 5/8] vhost: update_mem_cb implementation
From: "Dr. David Alan Gilbert"
Add the meat of update_mem_cb; this is called for each region,
to add a region to our temporary list.
Our temporary list is in order we look to see if this
region can be merged with the current head of list.
Signed-off-by: Dr. David Alan Gilbert
---
hw/virtio/trace-events | 2 ++
hw/virtio/vhost.c | 54 +-
2 files changed, 55 insertions(+), 1 deletion(-)
diff --git a/hw/virtio/trace-events b/hw/virtio/trace-events
index 4a493bcd46..92fadec192 100644
--- a/hw/virtio/trace-events
+++ b/hw/virtio/trace-events
@@ -2,6 +2,8 @@
# hw/virtio/vhost.c
vhost_section(const char *name, int r) "%s:%d"
+vhost_update_mem_cb(const char *name, uint64_t gpa, uint64_t size, uint64_t
host) "%s: 0x%"PRIx64"+0x%"PRIx64" @ 0x%"PRIx64
+vhost_update_mem_cb_abut(const char *name, uint64_t new_size) "%s: 0x%"PRIx64
# hw/virtio/virtio.c
virtqueue_alloc_element(void *elem, size_t sz, unsigned in_num, unsigned
out_num) "elem %p size %zd in_num %u out_num %u"
diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c
index d1b2d3441b..9d39271bfc 100644
--- a/hw/virtio/vhost.c
+++ b/hw/virtio/vhost.c
@@ -643,11 +643,63 @@ struct vhost_update_mem_tmp {
/* Called for each MRS from vhost_update_mem */
static int vhost_update_mem_cb(MemoryRegionSection *mrs, void *opaque)
{
+struct vhost_update_mem_tmp *vtmp = opaque;
+struct vhost_memory_region *cur_vmr;
+bool need_add = true;
+uint64_t mrs_size;
+uint64_t mrs_gpa;
+uintptr_t mrs_host;
+
if (!vhost_section(mrs)) {
return 0;
}
+mrs_size = int128_get64(mrs->size);
+mrs_gpa = mrs->offset_within_address_space;
+mrs_host = (uintptr_t)memory_region_get_ram_ptr(mrs->mr) +
+ mrs->offset_within_region;
+
+trace_vhost_update_mem_cb(mrs->mr->name, mrs_gpa, mrs_size, mrs_host);
+
+if (vtmp->nregions) {
+/* Since we already have at least one region, lets see if
+ * this extends it; since we're scanning in order, we only
+ * have to look at the last one, and the FlatView that calls
+ * us shouldn't have overlaps.
+ */
+struct vhost_memory_region *prev_vmr = vtmp->regions +
+ (vtmp->nregions - 1);
+uint64_t prev_gpa_start = prev_vmr->guest_phys_addr;
+uint64_t prev_gpa_end = range_get_last(prev_gpa_start,
+ prev_vmr->memory_size);
+uint64_t prev_host_start = prev_vmr->userspace_addr;
+uint64_t prev_host_end = range_get_last(prev_host_start,
+ prev_vmr->memory_size);
+
+if (prev_gpa_end + 1 == mrs_gpa &&
+prev_host_end + 1 == mrs_host &&
+(!vtmp->dev->vhost_ops->vhost_backend_can_merge ||
+vtmp->dev->vhost_ops->vhost_backend_can_merge(vtmp->dev,
+mrs_host, mrs_size,
+prev_host_start, prev_vmr->memory_size))) {
+/* The two regions abut */
+need_add = false;
+mrs_size = mrs_size + prev_vmr->memory_size;
+prev_vmr->memory_size = mrs_size;
+trace_vhost_update_mem_cb_abut(mrs->mr->name, mrs_size);
+}
+}
+
+if (need_add) {
+vtmp->nregions++;
+vtmp->regions = g_realloc_n(vtmp->regions, vtmp->nregions,
+sizeof(vtmp->regions[0]));
+cur_vmr = &vtmp->regions[vtmp->nregions - 1];
+cur_vmr->guest_phys_addr = mrs_gpa;
+cur_vmr->memory_size = mrs_size;
+cur_vmr->userspace_addr = mrs_host;
+cur_vmr->flags_padding = 0;
+}
-/* TODO */
return 0;
}
--
2.14.3
[Qemu-devel] [RFC v2 4/8] vhost: New memory update functions
From: "Dr. David Alan Gilbert"
vhost_update_mem will replace the existing update mechanism.
They make use of the Flatview we have now to make the update simpler.
This commit just adds the basic structure.
Signed-off-by: Dr. David Alan Gilbert
---
hw/virtio/vhost.c | 51 ++-
1 file changed, 50 insertions(+), 1 deletion(-)
diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c
index 8b2310a054..d1b2d3441b 100644
--- a/hw/virtio/vhost.c
+++ b/hw/virtio/vhost.c
@@ -634,11 +634,51 @@ static void vhost_begin(MemoryListener *listener)
dev->mem_changed_start_addr = -1;
}
+struct vhost_update_mem_tmp {
+struct vhost_dev *dev;
+uint32_t nregions;
+struct vhost_memory_region *regions;
+};
+
+/* Called for each MRS from vhost_update_mem */
+static int vhost_update_mem_cb(MemoryRegionSection *mrs, void *opaque)
+{
+if (!vhost_section(mrs)) {
+return 0;
+}
+
+/* TODO */
+return 0;
+}
+
+static int vhost_update_mem(struct vhost_dev *dev, bool *changed)
+{
+int res;
+struct vhost_update_mem_tmp vtmp;
+vtmp.regions = 0;
+vtmp.nregions = 0;
+vtmp.dev = dev;
+
+*changed = false;
+res = address_space_iterate(&address_space_memory,
+vhost_update_mem_cb, &vtmp);
+if (res) {
+goto out;
+}
+
+/* TODO */
+*changed = dev->mem_changed_start_addr > dev->mem_changed_end_addr;
+out:
+g_free(vtmp.regions);
+return res;
+}
+
static void vhost_commit(MemoryListener *listener)
{
struct vhost_dev *dev = container_of(listener, struct vhost_dev,
memory_listener);
uint64_t log_size;
+bool changed;
int r;
if (!dev->memory_changed) {
@@ -647,7 +687,12 @@ static void vhost_commit(MemoryListener *listener)
if (!dev->started) {
return;
}
-if (dev->mem_changed_start_addr > dev->mem_changed_end_addr) {
+if (vhost_update_mem(dev, &changed)) {
+return;
+}
+
+if (!changed) {
+/* None of the mappings we care about changed */
return;
}
@@ -1519,6 +1564,7 @@ void vhost_ack_features(struct vhost_dev *hdev, const int
*feature_bits,
int vhost_dev_start(struct vhost_dev *hdev, VirtIODevice *vdev)
{
int i, r;
+bool changed;
/* should only be called after backend is connected */
assert(hdev->vhost_ops);
@@ -1531,6 +1577,9 @@ int vhost_dev_start(struct vhost_dev *hdev, VirtIODevice
*vdev)
goto fail_features;
}
+if (vhost_update_mem(hdev, &changed)) {
+goto fail_mem;
+}
if (vhost_dev_has_iommu(hdev)) {
memory_listener_register(&hdev->iommu_listener, vdev->dma_as);
}
--
2.14.3
[Qemu-devel] [RFC v2 3/8] vhost: Simplify ring verification checks
From: "Dr. David Alan Gilbert"
vhost_verify_ring_mappings() were used to verify that
rings are still accessible and related memory hasn't
been moved after flatview is updated.
It were doing checks by mapping ring's GPA+len and
checking that HVA hasn't changed with new memory map.
To avoid maybe expensive mapping call, we were
identifying address range that changed and were doing
mapping only if ring were in changed range.
However it's no neccessary to perform ringi's GPA
mapping as we already have it's current HVA and all
we need is to verify that ring's GPA translates to
the same HVA in updated flatview.
This will allow the following patches to simplify the range
comparison that was previously needed to avoid expensive
verify_ring_mapping calls.
Signed-off-by: Igor Mammedov
with modifications by:
Signed-off-by: Dr. David Alan Gilbert
---
hw/virtio/vhost.c | 80 ++-
1 file changed, 44 insertions(+), 36 deletions(-)
diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c
index 938253d1e8..8b2310a054 100644
--- a/hw/virtio/vhost.c
+++ b/hw/virtio/vhost.c
@@ -450,35 +450,37 @@ static void vhost_memory_unmap(struct vhost_dev *dev,
void *buffer,
}
}
-static int vhost_verify_ring_part_mapping(struct vhost_dev *dev,
- void *part,
- uint64_t part_addr,
- uint64_t part_size,
- uint64_t start_addr,
- uint64_t size)
+static int vhost_verify_ring_part_mapping(void *ring_hva,
+ uint64_t ring_gpa,
+ uint64_t ring_size,
+ void *reg_hva,
+ uint64_t reg_gpa,
+ uint64_t reg_size)
{
-hwaddr l;
-void *p;
-int r = 0;
+uint64_t hva_ring_offset;
+uint64_t ring_last = range_get_last(ring_gpa, ring_size);
+uint64_t reg_last = range_get_last(reg_gpa, reg_size);
-if (!ranges_overlap(start_addr, size, part_addr, part_size)) {
+if (ring_last < reg_gpa || ring_gpa > reg_last) {
return 0;
}
-l = part_size;
-p = vhost_memory_map(dev, part_addr, &l, 1);
-if (!p || l != part_size) {
-r = -ENOMEM;
+/* check that whole ring's is mapped */
+if (ring_last > reg_last) {
+return -EBUSY;
}
-if (p != part) {
-r = -EBUSY;
+/* check that ring's MemoryRegion wasn't replaced */
+hva_ring_offset = ring_gpa - reg_gpa;
+if (ring_hva != reg_hva + hva_ring_offset) {
+return -ENOMEM;
}
-vhost_memory_unmap(dev, p, l, 0, 0);
-return r;
+
+return 0;
}
static int vhost_verify_ring_mappings(struct vhost_dev *dev,
- uint64_t start_addr,
- uint64_t size)
+ void *reg_hva,
+ uint64_t reg_gpa,
+ uint64_t reg_size)
{
int i, j;
int r = 0;
@@ -492,23 +494,26 @@ static int vhost_verify_ring_mappings(struct vhost_dev
*dev,
struct vhost_virtqueue *vq = dev->vqs + i;
j = 0;
-r = vhost_verify_ring_part_mapping(dev, vq->desc, vq->desc_phys,
- vq->desc_size, start_addr, size);
-if (!r) {
+r = vhost_verify_ring_part_mapping(
+vq->desc, vq->desc_phys, vq->desc_size,
+reg_hva, reg_gpa, reg_size);
+if (r) {
break;
}
j++;
-r = vhost_verify_ring_part_mapping(dev, vq->avail, vq->avail_phys,
- vq->avail_size, start_addr, size);
-if (!r) {
+r = vhost_verify_ring_part_mapping(
+vq->desc, vq->desc_phys, vq->desc_size,
+reg_hva, reg_gpa, reg_size);
+if (r) {
break;
}
j++;
-r = vhost_verify_ring_part_mapping(dev, vq->used, vq->used_phys,
- vq->used_size, start_addr, size);
-if (!r) {
+r = vhost_verify_ring_part_mapping(
+vq->desc, vq->desc_phys, vq->desc_size,
+reg_hva, reg_gpa, reg_size);
+if (r) {
break;
}
}
@@ -633,8 +638,6 @@ static void vhost_commit(MemoryListener *listener)
{
struct vhost_dev *dev = container_of(listener, struct vhost_dev,
memory_listener);
-hwaddr start_addr = 0;
-ram_addr_t size = 0;
uint64_t log_size;
int r;
@@ -649,11 +652,16 @@ static void vhost_commit(MemoryListener *listener)
}
if (dev->started) {
-start_addr = dev->mem_changed_start_addr;
-si
[Qemu-devel] [RFC v2 0/8] Rework vhost memory region updates
From: "Dr. David Alan Gilbert" Hi, This is an experimental set that reworks the way the vhost code handles changes in physical address space layout that came from a discussion with Igor. It's intention is to simplify a lot of the update code, and to make it easier for the postcopy+shared code to do the hugepage alignments that are needed. Instead of updating and trying to merge sections of address space on each add/remove callback, we wait until the commit phase and go through and rebuild a list by walking the Flatview of memory and end up producing an ordered list. We compare the list to the old list to trigger updates. This is pretty lightly tested (not least because I can't get vhost-user+hot-add memory to survive even upstream). v2 Incorporate changes and code from Igor, in particular the simpler ring verification checks and thus the removal of the complex comparison code. Dave Dr. David Alan Gilbert (8): memory: address_space_iterate vhost: Move log_dirty check vhost: Simplify ring verification checks vhost: New memory update functions vhost: update_mem_cb implementation vhost: Compare and copy updated region data into device state vhost: Remove old vhost_set_memory etc vhost: Move mem_sections maintenance into commit/update routines hw/virtio/trace-events| 7 + hw/virtio/vhost.c | 504 -- include/exec/memory.h | 23 +++ include/hw/virtio/vhost.h | 3 - memory.c | 22 ++ 5 files changed, 229 insertions(+), 330 deletions(-) -- 2.14.3
[Qemu-devel] [RFC v2 1/8] memory: address_space_iterate
From: "Dr. David Alan Gilbert"
Iterate through an address space calling a function for each
section. The iteration is done in order.
Signed-off-by: Dr. David Alan Gilbert
---
include/exec/memory.h | 23 +++
memory.c | 22 ++
2 files changed, 45 insertions(+)
diff --git a/include/exec/memory.h b/include/exec/memory.h
index 5ed4042f87..f5a9df642e 100644
--- a/include/exec/memory.h
+++ b/include/exec/memory.h
@@ -1987,6 +1987,29 @@ address_space_write_cached(MemoryRegionCache *cache,
hwaddr addr,
address_space_write(cache->as, cache->xlat + addr, MEMTXATTRS_UNSPECIFIED,
buf, len);
}
+/**
+ * ASIterateCallback: Function type called by address_space_iterate
+ *
+ * Return 0 on success or a negative error code.
+ *
+ * @mrs: Memory region section for this range
+ * @opaque: The opaque value passed in to the iterator.
+ */
+typedef int (*ASIterateCallback)(MemoryRegionSection *mrs, void *opaque);
+
+/**
+ * address_space_iterate: Call the function for each address range in the
+ *AddressSpace, in sorted order.
+ *
+ * Return 0 on success or a negative error code.
+ *
+ * @as: Address space to iterate over
+ * @cb: Function to call. If the function returns none-0 the iteration will
+ * stop.
+ * @opaque: Value to pass to the function
+ */
+int
+address_space_iterate(AddressSpace *as, ASIterateCallback cb, void *opaque);
#endif
#endif
diff --git a/memory.c b/memory.c
index e26e5a3b1d..f45137f25e 100644
--- a/memory.c
+++ b/memory.c
@@ -2810,6 +2810,28 @@ void address_space_destroy(AddressSpace *as)
call_rcu(as, do_address_space_destroy, rcu);
}
+int address_space_iterate(AddressSpace *as, ASIterateCallback cb,
+ void *opaque)
+{
+int res = 0;
+FlatView *fv = address_space_to_flatview(as);
+FlatRange *range;
+
+flatview_ref(fv);
+
+FOR_EACH_FLAT_RANGE(range, fv) {
+MemoryRegionSection mrs = section_from_flat_range(range, fv);
+res = cb(&mrs, opaque);
+if (res) {
+break;
+}
+}
+
+flatview_unref(fv);
+
+return res;
+}
+
static const char *memory_region_type(MemoryRegion *mr)
{
if (memory_region_is_ram_device(mr)) {
--
2.14.3
[Qemu-devel] [RFC v2 2/8] vhost: Move log_dirty check
From: "Dr. David Alan Gilbert"
Move the log_dirty check into vhost_section.
Signed-off-by: Dr. David Alan Gilbert
---
hw/virtio/trace-events | 3 +++
hw/virtio/vhost.c | 20 +---
2 files changed, 16 insertions(+), 7 deletions(-)
diff --git a/hw/virtio/trace-events b/hw/virtio/trace-events
index 775461ae98..4a493bcd46 100644
--- a/hw/virtio/trace-events
+++ b/hw/virtio/trace-events
@@ -1,5 +1,8 @@
# See docs/devel/tracing.txt for syntax documentation.
+# hw/virtio/vhost.c
+vhost_section(const char *name, int r) "%s:%d"
+
# hw/virtio/virtio.c
virtqueue_alloc_element(void *elem, size_t sz, unsigned in_num, unsigned
out_num) "elem %p size %zd in_num %u out_num %u"
virtqueue_fill(void *vq, const void *elem, unsigned int len, unsigned int idx)
"vq %p elem %p len %u idx %u"
diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c
index ddc42f0f93..938253d1e8 100644
--- a/hw/virtio/vhost.c
+++ b/hw/virtio/vhost.c
@@ -27,6 +27,7 @@
#include "hw/virtio/virtio-access.h"
#include "migration/blocker.h"
#include "sysemu/dma.h"
+#include "trace.h"
/* enabled until disconnected backend stabilizes */
#define _VHOST_DEBUG 1
@@ -567,18 +568,12 @@ static void vhost_set_memory(MemoryListener *listener,
memory_listener);
hwaddr start_addr = section->offset_within_address_space;
ram_addr_t size = int128_get64(section->size);
-bool log_dirty =
-memory_region_get_dirty_log_mask(section->mr) & ~(1 <<
DIRTY_MEMORY_MIGRATION);
int s = offsetof(struct vhost_memory, regions) +
(dev->mem->nregions + 1) * sizeof dev->mem->regions[0];
void *ram;
dev->mem = g_realloc(dev->mem, s);
-if (log_dirty) {
-add = false;
-}
-
assert(size);
/* Optimize no-change case. At least cirrus_vga does this a lot at this
time. */
@@ -611,8 +606,19 @@ static void vhost_set_memory(MemoryListener *listener,
static bool vhost_section(MemoryRegionSection *section)
{
-return memory_region_is_ram(section->mr) &&
+bool result;
+bool log_dirty = memory_region_get_dirty_log_mask(section->mr) &
+ ~(1 << DIRTY_MEMORY_MIGRATION);
+result = memory_region_is_ram(section->mr) &&
!memory_region_is_rom(section->mr);
+
+/* Vhost doesn't handle any block which is doing dirty-tracking other
+ * than migration; this typically fires on VGA areas.
+ */
+result &= !log_dirty;
+
+trace_vhost_section(section->mr->name, result);
+return result;
}
static void vhost_begin(MemoryListener *listener)
--
2.14.3
Re: [Qemu-devel] [Bug 1737194] Re: Windows NT 4.0 fails to boot from qcow2 installation
> On Dec 8, 2017, at 1:41 PM, John Snow <1737...@bugs.launchpad.net> wrote: > > If you do two identical installs with qcow2 and raw, do you see any > differences with qemu-img compare afterwards that might indicate what > could possibly have gone wrong? I think what you want is for me to compare a broken qcow2 file with a working qcow file. I'm not sure how to do that but if you sent me directions I can send you the results. I did try using the broken qcow2 file in a Windows XP VM. When I try to open the "Program Files" folder I see this error message: "D:\Program Files is not accessible. The file or directory is corrupted and unreadable". The WINNT folder does open. I tried checking the disk with Windows XP's disk repair utility. It failed to complete and displayed this error message: "Windows was unable to complete the disk check". > > -- > You received this bug notification because you are subscribed to the bug > report. > https://bugs.launchpad.net/bugs/1737194 > > Title: > Windows NT 4.0 fails to boot from qcow2 installation > > Status in QEMU: > New > > Bug description: > Windows NT 4.0 will not boot from an installation more than once if > installed in a qcow2 image file. A quick fix to this problem is to use > the qcow format instead. > > Steps to reproduce this issue: > > Create the image file: > qemu-img create -f qcow2 winnt4.qcow2 1G > > Boot from a Windows NT 4.0 Workstation CD: > qemu-system-i386 -hda winnt4.qcow2 -cdrom /dev/cdrom -boot d -m 128 -cpu > pentium -vga cirrus > > During the installation process you have the choise between FAT and > NTFS. You can pick anyone. > > After finishing the installation the guest will reboot to install > additional items. Once this is done the guest will be bootable. Eject > any CD media from QEMU and reboot. You will then see Windows NT 4.0 > booting up to the desktop. Go to "Start->Shut down" to shut down. Then > when Windows is ready quit QEMU. > > Now try to boot using this command: > qemu-system-i386 -hda winnt4.qcow2 -boot c -m 128 -cpu pentium -vga cirrus > > The BIOS screen will display an error message: > > For NTFS: > Booting from Hard Disk... > A disk read error occurred. > Insert a system diskette and restart > the system. > > For FAT: > No bootable device. > > Additional information: > qemu-system-i386 version: 2.10.1 > qemu-img version: 2.10.92 (v2.11.0-rc4-dirty) > > If you don't have a Windows NT 4.0 Workstation installation CD, you may > download one from here: > https://winworldpc.com/product/windows-nt-40/40 > > To manage notifications about this bug go to: > https://bugs.launchpad.net/qemu/+bug/1737194/+subscriptions -- You received this bug notification because you are a member of qemu- devel-ml, which is subscribed to QEMU. https://bugs.launchpad.net/bugs/1737194 Title: Windows NT 4.0 fails to boot from qcow2 installation Status in QEMU: New Bug description: Windows NT 4.0 will not boot from an installation more than once if installed in a qcow2 image file. A quick fix to this problem is to use the qcow format instead. Steps to reproduce this issue: Create the image file: qemu-img create -f qcow2 winnt4.qcow2 1G Boot from a Windows NT 4.0 Workstation CD: qemu-system-i386 -hda winnt4.qcow2 -cdrom /dev/cdrom -boot d -m 128 -cpu pentium -vga cirrus During the installation process you have the choise between FAT and NTFS. You can pick anyone. After finishing the installation the guest will reboot to install additional items. Once this is done the guest will be bootable. Eject any CD media from QEMU and reboot. You will then see Windows NT 4.0 booting up to the desktop. Go to "Start->Shut down" to shut down. Then when Windows is ready quit QEMU. Now try to boot using this command: qemu-system-i386 -hda winnt4.qcow2 -boot c -m 128 -cpu pentium -vga cirrus The BIOS screen will display an error message: For NTFS: Booting from Hard Disk... A disk read error occurred. Insert a system diskette and restart the system. For FAT: No bootable device. Additional information: qemu-system-i386 version: 2.10.1 qemu-img version: 2.10.92 (v2.11.0-rc4-dirty) If you don't have a Windows NT 4.0 Workstation installation CD, you may download one from here: https://winworldpc.com/product/windows-nt-40/40 To manage notifications about this bug go to: https://bugs.launchpad.net/qemu/+bug/1737194/+subscriptions
Re: [Qemu-devel] [PATCH 0/3] ide: abort TRIM operation for invalid range
On 12/08/2017 07:10 AM, Anton Nefedov wrote: > Started from the separate series discussion (trim statistics) , see > http://lists.nongnu.org/archive/html/qemu-devel/2017-12/msg01059.html > > There is no range check for IDE trim requests now. > Such request will likely be rejected by the block layer and count as > failed and not an invalid/aborted operation. > > Anton Nefedov (3): > ide: pass IDEState to trim AIO callback > ide: move ide_sect_range_ok() up > ide: abort TRIM operation for invalid range > > hw/ide/core.c | 53 + > 1 file changed, 33 insertions(+), 20 deletions(-) > Thanks, applied to my IDE tree: https://github.com/jnsnow/qemu/commits/ide https://github.com/jnsnow/qemu.git --js (PR won't be sent until after the 2.12 tree opens.)
Re: [Qemu-devel] [PATCH 2/5] lock-guard: add scoped lock implementation
On 12/08/2017 11:56 AM, Paolo Bonzini wrote:
>>> +typedef void QemuLockGuardFunc(void *);
>>> +typedef struct QemuLockGuard {
>>> +QemuLockGuardFunc *p_lock_fn, *p_unlock_fn;
>>> +void *lock;
>>> +int locked;
>>
>> bool?
>
> Yes.
>
>>> +#define QEMU_WITH_LOCK(type, name, lock)
>>> \
>>> +for (QEMU_LOCK_GUARD(type, name, lock);
>>> \
>>> + qemu_lock_guard_is_taken(&name);
>>> \
>>> + qemu_lock_guard_unlock(&name))
>>
>> I don't understand the need for the qemu_lock_guard_is_taken(&name)
>> condition, why not do the following?
>>
>> for (QEMU_LOCK_GUARD(type, name, lock);
>>;
>>qemu_lock_guard_unlock(&name))
>
> Because that would be an infinite loop. :)
Do we really need 'locked' to belong to the lockguard, just for our use
in for loops? Or can we just declare it in the for loop proper, as in
#define QEMU_WITH_LOCK(type, name, lock) \
for (bool name##_done = false, QEMU_LOCK_GUARD(type, name, lock); \
!name##_done; \
name##_done = true)
and let the automatic scope exit at the conclusion of the one-shot loop
thus unlock things on our behalf, and now we don't have to futz around
with guard->locked everywhere else?
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3266
Virtualization: qemu.org | libvirt.org
signature.asc
Description: OpenPGP digital signature
Re: [Qemu-devel] [PATCH v2] hw/ide: Remove duplicated definitions from ahci_internal.h
On 12/05/2017 02:10 AM, Thomas Huth wrote:
> The same definitions can also be found in include/hw/ide/ahci.h
> so let's remove these #defines from ahci_internal.h.
>
> Signed-off-by: Thomas Huth
> ---
> v2: Also remove TYPE_ICH9_AHCI as suggested by John
>
> hw/ide/ahci_internal.h | 12
> 1 file changed, 12 deletions(-)
>
> diff --git a/hw/ide/ahci_internal.h b/hw/ide/ahci_internal.h
> index ce2e818..e3e3ed2 100644
> --- a/hw/ide/ahci_internal.h
> +++ b/hw/ide/ahci_internal.h
> @@ -311,11 +311,6 @@ struct AHCIPCIState {
> AHCIState ahci;
> };
>
> -#define TYPE_ICH9_AHCI "ich9-ahci"
> -
> -#define ICH_AHCI(obj) \
> -OBJECT_CHECK(AHCIPCIState, (obj), TYPE_ICH9_AHCI)
> -
> extern const VMStateDescription vmstate_ahci;
>
> #define VMSTATE_AHCI(_field, _state) { \
> @@ -375,11 +370,4 @@ void ahci_uninit(AHCIState *s);
>
> void ahci_reset(AHCIState *s);
>
> -#define TYPE_SYSBUS_AHCI "sysbus-ahci"
> -#define SYSBUS_AHCI(obj) OBJECT_CHECK(SysbusAHCIState, (obj),
> TYPE_SYSBUS_AHCI)
> -
> -#define TYPE_ALLWINNER_AHCI "allwinner-ahci"
> -#define ALLWINNER_AHCI(obj) OBJECT_CHECK(AllwinnerAHCIState, (obj), \
> - TYPE_ALLWINNER_AHCI)
> -
> #endif /* HW_IDE_AHCI_H */
>
With edits:
Thanks, applied to my IDE tree:
https://github.com/jnsnow/qemu/commits/ide
https://github.com/jnsnow/qemu.git
--js
(PR will be sent when 2.12 opens; there's time to fiddle with it.)
Re: [Qemu-devel] [PATCH 5/5] thread-pool: convert to use lock guards
On 12/08/2017 12:12 PM, Paolo Bonzini wrote:
> On 08/12/2017 16:13, Stefan Hajnoczi wrote:
>>> -qemu_mutex_lock(&pool->lock);
>>> +QEMU_LOCK_GUARD(QemuMutex, pool_guard, &pool->lock);
>>> if (pool->idle_threads == 0 && pool->cur_threads < pool->max_threads) {
>>> spawn_thread(pool);
>>> }
>>> QTAILQ_INSERT_TAIL(&pool->request_list, req, reqs);
>>> -qemu_mutex_unlock(&pool->lock);
>>> +qemu_lock_guard_unlock(&pool_guard);
>> Why not QEMU_WITH_LOCK()? Then you can get rid of the explicit unlock.
>
> I agree that QEMU_WITH_LOCK_GUARD is better in this case. (IIRC I wrote
> this patch before coming up with the is_taken trick!).
>
> My main question for the series is what you think the balance should be
> between a more widely applicable API and a simpler one.
If you require the user to provide the scope, this could be:
@@ -258,12 +254,12 @@ BlockAIOCB *thread_pool_submit_aio(ThreadPool *pool,
trace_thread_pool_submit(pool, req, arg);
-qemu_mutex_lock(&pool->lock);
-if (pool->idle_threads == 0 && pool->cur_threads < pool->max_threads) {
-spawn_thread(pool);
-QTAILQ_INSERT_TAIL(&pool->request_list, req, reqs);
+{
+QEMU_LOCK_GUARD(QemuMutex, pool_guard, &pool->lock);
+if (pool->idle_threads == 0 && pool->cur_threads <
pool->max_threads) {
+spawn_thread(pool);
+}
+QTAILQ_INSERT_TAIL(&pool->request_list, req, reqs);
}
-qemu_mutex_unlock(&pool->lock);
qemu_sem_post(&pool->sem);
return &req->common;
}
In other words, I don't see what 'QEMU_WITH_LOCK_GUARD() {}' buys us
over '{ QEMU_LOCK_GUARD() }'.
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3266
Virtualization: qemu.org | libvirt.org
signature.asc
Description: OpenPGP digital signature
Re: [Qemu-devel] [PATCH 3/3] ide: abort TRIM operation for invalid range
On 12/08/2017 07:10 AM, Anton Nefedov wrote:
> ATA8-ACS3, 7.9 DATA SET MANAGEMENT - 06h, DMA
>
> 7.9.5 Error Outputs
> If the Trim bit is set to one and:
> a) the device detects an invalid LBA Range Entry; or
> b) count is greater than IDENTIFY DEVICE data word 105
> (see 7.16.7.55),
> then the device shall return command aborted.
> A device may trim one or more LBA Range Entries before it returns
> command aborted. See table 209.
>
> This check is not in the common ide_dma_cb() as the range for TRIM
> is harder to reach: it is not in LBA/count registers and the buffer has
> to be parsed first.
>
> Signed-off-by: Anton Nefedov
> ---
> hw/ide/core.c | 15 +--
> 1 file changed, 13 insertions(+), 2 deletions(-)
>
> diff --git a/hw/ide/core.c b/hw/ide/core.c
> index 23c71fa..3d1494f 100644
> --- a/hw/ide/core.c
> +++ b/hw/ide/core.c
> @@ -401,6 +401,7 @@ typedef struct TrimAIOCB {
> QEMUIOVector *qiov;
> BlockAIOCB *aiocb;
> int i, j;
> +bool is_invalid;
> } TrimAIOCB;
>
> static void trim_aio_cancel(BlockAIOCB *acb)
> @@ -428,8 +429,11 @@ static void ide_trim_bh_cb(void *opaque)
> {
> TrimAIOCB *iocb = opaque;
>
> -iocb->common.cb(iocb->common.opaque, iocb->ret);
> -
> +if (iocb->is_invalid) {
> +ide_dma_error(iocb->s);
> +} else {
> +iocb->common.cb(iocb->common.opaque, iocb->ret);
> +}
> qemu_bh_delete(iocb->bh);
> iocb->bh = NULL;
> qemu_aio_unref(iocb);
> @@ -456,6 +460,11 @@ static void ide_issue_trim_cb(void *opaque, int ret)
> continue;
> }
>
> +if (!ide_sect_range_ok(s, sector, count)) {
> +iocb->is_invalid = true;
> +goto done;
> +}
> +
> /* Got an entry! Submit and exit. */
> iocb->aiocb = blk_aio_pdiscard(s->blk,
> sector << BDRV_SECTOR_BITS,
> @@ -471,6 +480,7 @@ static void ide_issue_trim_cb(void *opaque, int ret)
> iocb->ret = ret;
> }
>
> +done:
> iocb->aiocb = NULL;
> if (iocb->bh) {
> qemu_bh_schedule(iocb->bh);
> @@ -491,6 +501,7 @@ BlockAIOCB *ide_issue_trim(
> iocb->qiov = qiov;
> iocb->i = -1;
> iocb->j = 0;
> +iocb->is_invalid = false;
> ide_issue_trim_cb(iocb, 0);
> return &iocb->common;
> }>
Looks about right, just remember that this flow won't call
block_acct_invalid because you're bypassing the return to ide_dma_cb. I
assume you'll get to that in your next series.
For now, this should properly reject bogus TRIM commands. When you send
your next series, may I ask for a simple test case if possible?
1-3:
Reviewed-by: John Snow
Re: [Qemu-devel] [PATCH 5/5] thread-pool: convert to use lock guards
On 12/08/2017 09:13 AM, Stefan Hajnoczi wrote:
> On Fri, Dec 08, 2017 at 11:55:53AM +0100, Paolo Bonzini wrote:
>> @@ -88,9 +88,9 @@ static void *worker_thread(void *opaque)
>>
>> do {
>> pool->idle_threads++;
>> -qemu_mutex_unlock(&pool->lock);
>> +qemu_lock_guard_unlock(&pool_guard);
>> ret = qemu_sem_timedwait(&pool->sem, 1);
>> -qemu_mutex_lock(&pool->lock);
>> +qemu_lock_guard_lock(&pool_guard);
>
> Or:
>
> QEMU_WITHOUT_LOCK_GUARD(pool_guard) {
> ret = qemu_sem_timedwait(&pool->sem, 1);
> }
>
> Seems funny but I like it. :)
>
> Unfortunately it's tricky to come up with good semantics. A 'return'
> statement inside QEMU_WITHOUT_LOCK_GUARD() should leave the lock
> unlocked. But a 'break' statement may need to reacquire the lock...
But isn't that what happens already?
QEMU_WITH_LOCK_GUARD(guard) { // 1
do {
QEMU_WITHOUT_LOCK_GUARD(guard) {// 2
if (cond1) {
break;
} else if (cond2) {
goto endlock;
} else {
return;
}
} // 3
} while (cond3);
} // 4
endlock:
; // 5
Point 2 introduces a new scope which says to unlock the guard at entry,
and to relock the guard at all exits from the scope. If cond1 is true,
and we break, control flows to point 3, (because of we provided the
scope via a for loop - so the break exits only the lock guard!), and
relocks the guard, so the bulk of the do/while loop that is not
sub-scoped by the WITHOUT_LOCK_GUARD remains locked. If cond2 is true,
then we leave the WITHOUT_LOCK_GUARD scope (and temporarily lock the
guard), then immediately leave the WITH_LOCK_GUARD scope (and unlock the
guard again) - the cleanup handlers ensure that unwinding out of
multiple scopes performs all of the proper cleanups, as we skip to point
5. The same is true if cond2 is false and we return from the function -
the lock is temporarily restored then released.
Or did you want semantics where you can return with the lock still
locked in spite of the outer loop, and/or an optimization to avoid the
contention on temporarily re-locking/unlocking a loop when jumping out
of two nested scopes?
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3266
Virtualization: qemu.org | libvirt.org
signature.asc
Description: OpenPGP digital signature
Re: [Qemu-devel] [RFC PATCH 0/5] Scoped locks using attribute((cleanup))
On 12/08/2017 04:55 AM, Paolo Bonzini wrote:
> This is an attempt to make a C API that resembles the C++
> std::unique_lock (mostly untested). The idea is that you can write
>
> QEMU_LOCK_GUARD(QemuMutex, guard_name, &some_mutex);
>
> instead of
>
> qemu_mutex_lock(&some_mutex);
> ...
> out:
> qemu_mutex_unlock(&some_mutex);
>
> and the mutex will be unlocked on all exit paths. In C++ that
> would be "std::unique_lock guard_name(some_mutex);".
> Likewise,
>
> QEMU_WITH_LOCK(QemuMutex, guard_name, &some_mutex) {
> ...
> }
>
> is the same as
>
> qemu_mutex_lock(&some_mutex);
> ...
> qemu_mutex_unlock(&some_mutex);
>
> except that any returns within the region will unlock the mutex.
Not just returns, but ANY manner that you leave the scope, including a
goto or just falling out of the end of the scope.
(and, as Stefan pointed out, this poisons the use of 'break' when this
is used inside a loop, as it now breaks the scope of the QEMU_WITH_LOCK
instead of the outer loop).
> It's possible to use QemuLockGuard also with a spinlock or a
> CoMutex. However, it is _not_ possible to return a QemuLockGuard
> since C doesn't have an equivalent of C++'s "move semantics", so
> there are other "constructor" macros such as QEMU_ADOPT_LOCK
> and QEMU_TAKEN_LOCK.
>
> On the positive side, I checked that the entire abstraction
> is removed by the compiler, the generated code is more or less
> the same. Also, it would let us for example change block/curl.c
> to use a CoQueue instead of a home-grown list+QemuMutex.
>
> However, I am still not sure about the readability, and it doesn't play
> well with another common idiom in QEMU, which is to wrap global mutexes
> with function such as
>
> static void block_job_lock(void)
> {
> qemu_mutex_lock(&block_job_mutex);
> }
>
> static void block_job_unlock(void)
> {
> qemu_mutex_unlock(&block_job_mutex);
> }
>
> So I'm a bit underwhelmed by this experiment. Other opinions?
The semantics you list here:
> QEMU_WITH_LOCK(QemuMutex, guard_name, &some_mutex) {
> ...
> }
are slightly different from the semantics in nbdkit:
{
ACQUIRE_LOCK_FOR_CURRENT_SCOPE(&some_lock);
...
}
In that your version requires the user to supply the scope after your
macro, instead of using your macro within the scope. But I guess that's
because you added other helpers like QEMU_WITH_ADOPTED_LOCK,
QEMU_RETURN_LOCK, and QEMU_TAKEN_LOCK, where you have multiple
possibilities for which state the lock should be in.
Is it worth playing around with the user providing the scope around your
macros, instead of using your macro to introduce the scope?
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3266
Virtualization: qemu.org | libvirt.org
signature.asc
Description: OpenPGP digital signature
Re: [Qemu-devel] [PATCH 03/12] build-sys: add a rule to print a variable
On 12/07/2017 06:58 PM, Marc-André Lureau wrote: > $ make print-CFLAGS > CFLAGS=-fsanitize=address -Og -g > > Trick from various sources: > https://stackoverflow.com/questions/16467718/how-to-print-out-a-variable-in-makefile > https://www.cmcrossroads.com/article/printing-value-makefile-variable > > Signed-off-by: Marc-André Lureau > --- > Makefile | 5 - > 1 file changed, 4 insertions(+), 1 deletion(-) > Slick! Does it need to be documented anywhere that we have this useful debugging aid in place? Reviewed-by: Eric Blake > diff --git a/Makefile b/Makefile > index ab0354c153..80683e8c8b 100644 > --- a/Makefile > +++ b/Makefile > @@ -6,7 +6,10 @@ BUILD_DIR=$(CURDIR) > # Before including a proper config-host.mak, assume we are in the source tree > SRC_PATH=. > > -UNCHECKED_GOALS := %clean TAGS cscope ctags docker docker-% help > +UNCHECKED_GOALS := %clean TAGS cscope ctags docker docker-% help print-% > + > +print-%: > + @echo '$*=$($*)' > > # All following code might depend on configuration variables > ifneq ($(wildcard config-host.mak),) > -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org signature.asc Description: OpenPGP digital signature
Re: [Qemu-devel] [PATCH 02/12] build-sys: silence make by default
On 12/07/2017 06:58 PM, Marc-André Lureau wrote: > In particular, do not print anything when there is nothing to do, in > particular, after a successful build: Do we need both 'in particular'? > > $ make > make[1]: '/home/elmarco/src/qemu/build/capstone/libcapstone.a' is up to date. > > Signed-off-by: Marc-André Lureau > --- > rules.mak | 5 + > 1 file changed, 5 insertions(+) Yay! I've been bothered by this. You may want to add that the noise is conditional on whether you have capstone-devel installed. However, > > diff --git a/rules.mak b/rules.mak > index 6e943335f3..b760d54908 100644 > --- a/rules.mak > +++ b/rules.mak > @@ -131,6 +131,11 @@ modules: > # If called with only a single argument, will print nothing in quiet mode. > quiet-command = $(if $(V),$1,$(if $(2),@printf " %-7s %s\n" $2 $3 && $1, > @$1)) > > +makeflags_ = $(makeflags_0) > +makeflags_0 = --no-print-directory -s Why the mix of long option and short option? Would it be more legible to use two long options? In my testing of your patch, '-s' alone also silenced things (admittedly, when libcapstone.a is up-to-date). What is --no-print-directory adding? /me goes digging Hmm, we already have this in Makefile: SUBDIR_MAKEFLAGS=$(if $(V),,--no-print-directory) BUILD_DIR=$(BUILD_DIR) So maybe our problem is that we are forgetting to feed SUBDIR_MAKEFLAGS to our sub-make that builds libcapstone.a? Next, why is -s silencing the "is up to date" message? Per 'make --help', -s controls whether recipes are echoed - but that doesn't seem to be a recipe that was echoed. Furthermore, the make manual mentions that -s is automatically added to MAKEFLAGS if it was present in the command line; are we accidentally silencing too much (if the user wants to do 'make -s V=1' to get JUST command lines but not the make noise)? > +makeflags_1 = > +MAKEFLAGS += $(makeflags_$(V)) > + So I'm not yet convinced that this is the right place, or if it is too heavy of a hammer. -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org signature.asc Description: OpenPGP digital signature
Re: [Qemu-devel] [Qemu-block] [PATCH] iotests: Tweak 030 in order to trigger a race condition with parallel jobs
On 12/07/2017 02:34 PM, Alberto Garcia wrote:
> On Thu 07 Dec 2017 08:16:41 PM CET, Eric Blake wrote:
>>> qemu_io('-f', iotests.imgfmt,
>>> -'-c', 'write -P %d %d %d' % (i, i*1024*1024, num_kb *
>>> 1024),
>>> +'-c', 'write -P 0xFF %dk %dk' % (i * 512, num_kb),
>>
>> I guess changing from a variable to a fixed 0xff pattern doesn't make
>> a difference?
>
> I noticed that with the previous code we would write zeroes to the first
> image (i == 0), and with that I can't reproduce the bug. I assume that
> block-stream doesn't copy the data in that case. Changing it to anything
> != 0 solves the problem.
>
I think I ran into a similar problem with an AHCI test once.
Reviewed-by: John Snow
> And answering your question, it doesn't really matter if we write the
> same value in all places, we only check the output of 'qemu-io -c map'.
> Plus the areas don't even overlap.
>
> Berto
>
Re: [Qemu-devel] [Qemu-block] [PATCH 2/2] qcow2: fix indentation after previous patch
On 11/30/2017 11:47 AM, Vladimir Sementsov-Ogievskiy wrote:
> Signed-off-by: Vladimir Sementsov-Ogievskiy
> ---
> block/qcow2.h | 34 +-
> block/qcow2.c | 16
> 2 files changed, 25 insertions(+), 25 deletions(-)
>
> diff --git a/block/qcow2.h b/block/qcow2.h
> index 8f226a3609..896ad08e5b 100644
> --- a/block/qcow2.h
> +++ b/block/qcow2.h
> @@ -399,29 +399,29 @@ typedef enum QCow2ClusterType {
> } QCow2ClusterType;
>
> typedef enum QCow2MetadataOverlap {
> -QCOW2_OL_MAIN_HEADER_BITNR= 0,
> -QCOW2_OL_ACTIVE_L1_BITNR = 1,
> -QCOW2_OL_ACTIVE_L2_BITNR = 2,
> -QCOW2_OL_REFCOUNT_TABLE_BITNR = 3,
> -QCOW2_OL_REFCOUNT_BLOCK_BITNR = 4,
> -QCOW2_OL_SNAPSHOT_TABLE_BITNR = 5,
> -QCOW2_OL_INACTIVE_L1_BITNR= 6,
> -QCOW2_OL_INACTIVE_L2_BITNR= 7,
> +QCOW2_OL_MAIN_HEADER_BITNR = 0,
> +QCOW2_OL_ACTIVE_L1_BITNR= 1,
> +QCOW2_OL_ACTIVE_L2_BITNR= 2,
> +QCOW2_OL_REFCOUNT_TABLE_BITNR = 3,
> +QCOW2_OL_REFCOUNT_BLOCK_BITNR = 4,
> +QCOW2_OL_SNAPSHOT_TABLE_BITNR = 5,
> +QCOW2_OL_INACTIVE_L1_BITNR = 6,
> +QCOW2_OL_INACTIVE_L2_BITNR = 7,
> QCOW2_OL_BITMAP_DIRECTORY_BITNR = 8,
>
> QCOW2_OL_MAX_BITNR = 9,
>
> -QCOW2_OL_NONE = 0,
> -QCOW2_OL_MAIN_HEADER= (1 << QCOW2_OL_MAIN_HEADER_BITNR),
> -QCOW2_OL_ACTIVE_L1 = (1 << QCOW2_OL_ACTIVE_L1_BITNR),
> -QCOW2_OL_ACTIVE_L2 = (1 << QCOW2_OL_ACTIVE_L2_BITNR),
> -QCOW2_OL_REFCOUNT_TABLE = (1 << QCOW2_OL_REFCOUNT_TABLE_BITNR),
> -QCOW2_OL_REFCOUNT_BLOCK = (1 << QCOW2_OL_REFCOUNT_BLOCK_BITNR),
> -QCOW2_OL_SNAPSHOT_TABLE = (1 << QCOW2_OL_SNAPSHOT_TABLE_BITNR),
> -QCOW2_OL_INACTIVE_L1= (1 << QCOW2_OL_INACTIVE_L1_BITNR),
> +QCOW2_OL_NONE = 0,
> +QCOW2_OL_MAIN_HEADER = (1 << QCOW2_OL_MAIN_HEADER_BITNR),
> +QCOW2_OL_ACTIVE_L1= (1 << QCOW2_OL_ACTIVE_L1_BITNR),
> +QCOW2_OL_ACTIVE_L2= (1 << QCOW2_OL_ACTIVE_L2_BITNR),
> +QCOW2_OL_REFCOUNT_TABLE = (1 << QCOW2_OL_REFCOUNT_TABLE_BITNR),
> +QCOW2_OL_REFCOUNT_BLOCK = (1 << QCOW2_OL_REFCOUNT_BLOCK_BITNR),
> +QCOW2_OL_SNAPSHOT_TABLE = (1 << QCOW2_OL_SNAPSHOT_TABLE_BITNR),
> +QCOW2_OL_INACTIVE_L1 = (1 << QCOW2_OL_INACTIVE_L1_BITNR),
> /* NOTE: Checking overlaps with inactive L2 tables will result in bdrv
> * reads. */
> -QCOW2_OL_INACTIVE_L2= (1 << QCOW2_OL_INACTIVE_L2_BITNR),
> +QCOW2_OL_INACTIVE_L2 = (1 << QCOW2_OL_INACTIVE_L2_BITNR),
> QCOW2_OL_BITMAP_DIRECTORY = (1 << QCOW2_OL_BITMAP_DIRECTORY_BITNR),
> } QCow2MetadataOverlap;
>
> diff --git a/block/qcow2.c b/block/qcow2.c
> index 8278c0e124..23f36a67c7 100644
> --- a/block/qcow2.c
> +++ b/block/qcow2.c
> @@ -687,14 +687,14 @@ static QemuOptsList qcow2_runtime_opts = {
> };
>
> static const char *overlap_bool_option_names[QCOW2_OL_MAX_BITNR] = {
> -[QCOW2_OL_MAIN_HEADER_BITNR]= QCOW2_OPT_OVERLAP_MAIN_HEADER,
> -[QCOW2_OL_ACTIVE_L1_BITNR] = QCOW2_OPT_OVERLAP_ACTIVE_L1,
> -[QCOW2_OL_ACTIVE_L2_BITNR] = QCOW2_OPT_OVERLAP_ACTIVE_L2,
> -[QCOW2_OL_REFCOUNT_TABLE_BITNR] = QCOW2_OPT_OVERLAP_REFCOUNT_TABLE,
> -[QCOW2_OL_REFCOUNT_BLOCK_BITNR] = QCOW2_OPT_OVERLAP_REFCOUNT_BLOCK,
> -[QCOW2_OL_SNAPSHOT_TABLE_BITNR] = QCOW2_OPT_OVERLAP_SNAPSHOT_TABLE,
> -[QCOW2_OL_INACTIVE_L1_BITNR]= QCOW2_OPT_OVERLAP_INACTIVE_L1,
> -[QCOW2_OL_INACTIVE_L2_BITNR]= QCOW2_OPT_OVERLAP_INACTIVE_L2,
> +[QCOW2_OL_MAIN_HEADER_BITNR] = QCOW2_OPT_OVERLAP_MAIN_HEADER,
> +[QCOW2_OL_ACTIVE_L1_BITNR]= QCOW2_OPT_OVERLAP_ACTIVE_L1,
> +[QCOW2_OL_ACTIVE_L2_BITNR]= QCOW2_OPT_OVERLAP_ACTIVE_L2,
> +[QCOW2_OL_REFCOUNT_TABLE_BITNR] = QCOW2_OPT_OVERLAP_REFCOUNT_TABLE,
> +[QCOW2_OL_REFCOUNT_BLOCK_BITNR] = QCOW2_OPT_OVERLAP_REFCOUNT_BLOCK,
> +[QCOW2_OL_SNAPSHOT_TABLE_BITNR] = QCOW2_OPT_OVERLAP_SNAPSHOT_TABLE,
> +[QCOW2_OL_INACTIVE_L1_BITNR] = QCOW2_OPT_OVERLAP_INACTIVE_L1,
> +[QCOW2_OL_INACTIVE_L2_BITNR] = QCOW2_OPT_OVERLAP_INACTIVE_L2,
> [QCOW2_OL_BITMAP_DIRECTORY_BITNR] = QCOW2_OPT_OVERLAP_BITMAP_DIRECTORY,
> };
>
>
Squash it in, or don't, either way:
Reviewed-by: John Snow
[Qemu-devel] [Bug 1737194] Re: Windows NT 4.0 fails to boot from qcow2 installation
If you do two identical installs with qcow2 and raw, do you see any differences with qemu-img compare afterwards that might indicate what could possibly have gone wrong? -- You received this bug notification because you are a member of qemu- devel-ml, which is subscribed to QEMU. https://bugs.launchpad.net/bugs/1737194 Title: Windows NT 4.0 fails to boot from qcow2 installation Status in QEMU: New Bug description: Windows NT 4.0 will not boot from an installation more than once if installed in a qcow2 image file. A quick fix to this problem is to use the qcow format instead. Steps to reproduce this issue: Create the image file: qemu-img create -f qcow2 winnt4.qcow2 1G Boot from a Windows NT 4.0 Workstation CD: qemu-system-i386 -hda winnt4.qcow2 -cdrom /dev/cdrom -boot d -m 128 -cpu pentium -vga cirrus During the installation process you have the choise between FAT and NTFS. You can pick anyone. After finishing the installation the guest will reboot to install additional items. Once this is done the guest will be bootable. Eject any CD media from QEMU and reboot. You will then see Windows NT 4.0 booting up to the desktop. Go to "Start->Shut down" to shut down. Then when Windows is ready quit QEMU. Now try to boot using this command: qemu-system-i386 -hda winnt4.qcow2 -boot c -m 128 -cpu pentium -vga cirrus The BIOS screen will display an error message: For NTFS: Booting from Hard Disk... A disk read error occurred. Insert a system diskette and restart the system. For FAT: No bootable device. Additional information: qemu-system-i386 version: 2.10.1 qemu-img version: 2.10.92 (v2.11.0-rc4-dirty) If you don't have a Windows NT 4.0 Workstation installation CD, you may download one from here: https://winworldpc.com/product/windows-nt-40/40 To manage notifications about this bug go to: https://bugs.launchpad.net/qemu/+bug/1737194/+subscriptions
Re: [Qemu-devel] [Qemu-arm] [PATCH] hw/arm/virt: support 4 serial ports
On Fri, Dec 8, 2017 at 12:49 PM, Peter Maydell wrote: > Now that the 2.11 release is mostly out of the way, I've been > working through some of my other todo list items so I've > come back to this thread. > > My suggestion is that we should add a second non-secure UART > to the virt board, so if you're using secure=no then you > get serial 1 and 2, and for secure=yes serial 1 is the first > NS uart, serial 2 is the secure uart and serial 3 is the > 2nd NS uart. (I don't really want to add a 4th uart unless > there's some good reason to -- maybe we would turn out to > want it for the secure side, for instance.) > > I'll probably write a patch for this next week, unless you > want to do it. Sounds great to me. Thanks! I'm curious why you don't want to add a 4th UART. It seems trivial to do, and the more the merrier? Or that's a silly attitude? Mostly just curious about that, as having two personally suits my needs.
Re: [Qemu-devel] [PATCH 1/2] tpm_spapr: Support TPM for ppc64 using CRQ based interface
On 12/06/2017 12:53 PM, Stefan Berger wrote:
> On 12/06/2017 02:26 AM, Cédric Le Goater wrote:
>> Hello Stefan,
>>
>> Some comments below. How do we populate the device tree ?
>
> Patched SLOF.
Ah. Isn't that a problem ? I thought QEMU was in charge of populating
the device tree.
>>
>> Thanks,
>
> Thanks for the review. I will process the comments. I have some responses
> below.
>
> One thing I wasn't sure about is this here:
>
> //k->devnode = tpm_spapr_devnode;
>
>
> Is a function for this needed? If not, I will remove.
If you don't define one, it won't be used. I think this is old API
to populate the DT which is not used anymore. To be checked I might
be wrong.
>>
>> On 12/05/2017 09:35 PM, Stefan Berger wrote:
>>> Implement support for TPM on ppc64 by implementing the vTPM CRQ
>>> interface as a frontend.
>>>
>>> The Linux vTPM driver for ppc64 works with this emulation.
>>>
>>> This emualtor also handles the TPM 2 case.
>>>
>>> Signed-off-by: Stefan Berger
>>> ---
>>> hw/tpm/Makefile.objs | 1 +
>>> hw/tpm/tpm_spapr.c | 380
>>> +++
>>> include/sysemu/tpm.h | 3 +
>>> qapi/tpm.json | 5 +-
>>> 4 files changed, 387 insertions(+), 2 deletions(-)
>>> create mode 100644 hw/tpm/tpm_spapr.c
>>>
>>> diff --git a/hw/tpm/Makefile.objs b/hw/tpm/Makefile.objs
>>> index 41f0b7a..71ea63e 100644
>>> --- a/hw/tpm/Makefile.objs
>>> +++ b/hw/tpm/Makefile.objs
>>> @@ -1,3 +1,4 @@
>>> common-obj-$(CONFIG_TPM_TIS) += tpm_tis.o
>>> common-obj-$(CONFIG_TPM_PASSTHROUGH) += tpm_passthrough.o tpm_util.o
>>> common-obj-$(CONFIG_TPM_EMULATOR) += tpm_emulator.o tpm_util.o
>>> +obj-$(CONFIG_PSERIES) += tpm_spapr.o
>>> diff --git a/hw/tpm/tpm_spapr.c b/hw/tpm/tpm_spapr.c
>>> new file mode 100644
>>> index 000..909aeeb
>>> --- /dev/null
>>> +++ b/hw/tpm/tpm_spapr.c
>>> @@ -0,0 +1,380 @@
>>> +/*
>>> + * QEMU PowerPC pSeries Logical Partition (aka sPAPR) hardware System
>>> Emulator
>>> + *
>>> + * PAPR Virtual TPM
>>> + *
>>> + * Copyright (c) 2015, 2017 IBM Corporation.
>>> + *
>>> + * Authors:
>>> + * Stefan Berger
>>> + *
>>> + *
>>> + * Permission is hereby granted, free of charge, to any person obtaining a
>>> copy
>>> + * of this software and associated documentation files (the "Software"),
>>> to deal
>>> + * in the Software without restriction, including without limitation the
>>> rights
>>> + * to use, copy, modify, merge, publish, distribute, sublicense, and/or
>>> sell
>>> + * copies of the Software, and to permit persons to whom the Software is
>>> + * furnished to do so, subject to the following conditions:
>>> + *
>>> + * The above copyright notice and this permission notice shall be included
>>> in
>>> + * all copies or substantial portions of the Software.
>>> + *
>>> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS
>>> OR
>>> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
>>> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
>>> + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR
>>> OTHER
>>> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
>>> FROM,
>>> + * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS
>>> IN
>>> + * THE SOFTWARE.
>> May be reduce a bit the header.
>
> Which part? I derived the header from spapr_vscsi.c ...
This should be enough :
/*
*
*
* Copyright (c) 2015-2017, IBM Corporation.
*
* This code is licensed under the GPL version 2 or later. See the
* COPYING file in the top-level directory.
*/
>
>>
>>> + */
>>> +
>>> +#include "qemu/osdep.h"
>>> +#include "qapi/error.h"
>>> +
>>> +#include "sysemu/tpm_backend.h"
>>> +#include "tpm_int.h"
>>> +#include "tpm_util.h"
>>> +
>>> +#include "hw/ppc/spapr.h"
>>> +#include "hw/ppc/spapr_vio.h"
>>> +
>>> +#define DEBUG_SPAPR_VTPM 0
>>> +
>>> +#define DPRINTF(fmt, ...) do { \
>>> + if (DEBUG_SPAPR_VTPM) { \
>>> + printf(fmt, ## __VA_ARGS__); \
>>> + } \
>>> +} while (0)
>> traces are prefered over printfs
>>
>>> +#define VIO_SPAPR_VTPM_DEVICE(obj) \
>> I think you can trim the _DEVICE
>
> Not sure what you mean.
I think VIO_SPAPR_VTPM is fine.
>> .
>>
>>> + OBJECT_CHECK(SPAPRvTPMState, (obj), TYPE_TPM_SPAPR)
>>> +
>>> +typedef struct vio_crq {
>> type definitions should always use CamelCase.
>
> Will fix. Derived from vscsi...
>
>>
>>> + uint8_t valid; /* 0x80: cmd; 0xc0: init crq
>>> + 0x81-0x83: CRQ message response */
>>> + uint8_t msg; /* see below */
>>> + uint16_t len; /* len of TPM request; len of TPM response */
>>> + uint32_t data; /* rtce_dma_handle when sending TPM request */
>>> + uint64_t reserved;
>>> +} vio_crq;
>>> +
>>> +typedef union tpm_spapr_crq {
>> ditto.
>>
>>> + vio_crq s;
>>> + uint8_t raw[sizeof(vio_crq)];
>>> +} tpm_spapr_crq;
>>> +
>>> +#define SPAPR_VTPM_VALID_INIT_CRQ_COMMAND
Re: [Qemu-devel] [PATCH 5/5] thread-pool: convert to use lock guards
On 08/12/2017 16:13, Stefan Hajnoczi wrote:
>> -qemu_mutex_lock(&pool->lock);
>> +QEMU_LOCK_GUARD(QemuMutex, pool_guard, &pool->lock);
>> if (pool->idle_threads == 0 && pool->cur_threads < pool->max_threads) {
>> spawn_thread(pool);
>> }
>> QTAILQ_INSERT_TAIL(&pool->request_list, req, reqs);
>> -qemu_mutex_unlock(&pool->lock);
>> +qemu_lock_guard_unlock(&pool_guard);
> Why not QEMU_WITH_LOCK()? Then you can get rid of the explicit unlock.
I agree that QEMU_WITH_LOCK_GUARD is better in this case. (IIRC I wrote
this patch before coming up with the is_taken trick!).
My main question for the series is what you think the balance should be
between a more widely applicable API and a simpler one.
Thanks,
Paolo
>> @@ -330,7 +326,7 @@ void thread_pool_free(ThreadPool *pool)
>>
>> assert(QLIST_EMPTY(&pool->head));
>>
>> -qemu_mutex_lock(&pool->lock);
>> +QEMU_LOCK_GUARD(QemuMutex, pool_guard, &pool->lock);
>>
>> /* Stop new threads from spawning */
>> qemu_bh_delete(pool->new_thread_bh);
>> @@ -344,7 +340,7 @@ void thread_pool_free(ThreadPool *pool)
>> qemu_cond_wait(&pool->worker_stopped, &pool->lock);
>> }
>>
>> -qemu_mutex_unlock(&pool->lock);
>> +qemu_lock_guard_unlock(&pool_guard);
> Here too. I don't see the advantage of replacing a single lock/unlock
> with QEMU_LOCK_GUARD/unlock, if it cannot be made shorter/safer then
> it's fine to use QemuMutex directly.
Re: [Qemu-devel] [PATCH 11/12] tests: fix qmp-test leak
Marc-André Lureau writes:
> Direct leak of 913 byte(s) in 43 object(s) allocated from:
> #0 0x55880a15df60 in __interceptor_malloc
> (/home/elmarco/src/qq/build/tests/qmp-test+0x110f60)
> #1 0x7f3f20fd098f in _IO_vasprintf (/lib64/libc.so.6+0x8098f)
>
> Signed-off-by: Marc-André Lureau
> ---
> tests/qmp-test.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/tests/qmp-test.c b/tests/qmp-test.c
> index c5a5c10b41..36feb2204b 100644
> --- a/tests/qmp-test.c
> +++ b/tests/qmp-test.c
> @@ -271,7 +271,7 @@ static void add_query_tests(QmpSchema *schema)
> {
> SchemaInfoList *tail;
> SchemaInfo *si, *arg_type, *ret_type;
> -const char *test_name;
> +char *test_name;
>
> /* Test the query-like commands */
> for (tail = schema->list; tail; tail = tail->next) {
> @@ -297,6 +297,7 @@ static void add_query_tests(QmpSchema *schema)
>
> test_name = g_strdup_printf("qmp/%s", si->name);
> qtest_add_data_func(test_name, si->name, test_query);
> +g_free(test_name);
> }
> }
Reviewed-by: Markus Armbruster
Re: [Qemu-devel] [PATCH 05/12] tests: fix check-qobject leak:
Marc-André Lureau writes: > /public/qobject_is_equal_conversion: OK > > = > ==14396==ERROR: LeakSanitizer: detected memory leaks > > Direct leak of 56 byte(s) in 1 object(s) allocated from: > #0 0x7f07682c5850 in malloc (/lib64/libasan.so.4+0xde850) > #1 0x7f0767d12f0c in g_malloc ../glib/gmem.c:94 > #2 0x7f0767d131cf in g_malloc_n ../glib/gmem.c:331 > #3 0x562bd767371f in do_test_equality > /home/elmarco/src/qq/tests/check-qobject.c:49 > #4 0x562bd7674a35 in qobject_is_equal_dict_test > /home/elmarco/src/qq/tests/check-qobject.c:267 > #5 0x7f0767d37b04 in test_case_run ../glib/gtestutils.c:2237 > #6 0x7f0767d37ec4 in g_test_run_suite_internal ../glib/gtestutils.c:2321 > #7 0x7f0767d37f6d in g_test_run_suite_internal ../glib/gtestutils.c:2333 > #8 0x7f0767d38184 in g_test_run_suite ../glib/gtestutils.c:2408 > #9 0x7f0767d36e0d in g_test_run ../glib/gtestutils.c:1674 > #10 0x562bd7674e75 in main /home/elmarco/src/qq/tests/check-qobject.c:327 > #11 0x7f0766009039 in __libc_start_main (/lib64/libc.so.6+0x21039) > > Signed-off-by: Marc-André Lureau > --- > tests/check-qobject.c | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/tests/check-qobject.c b/tests/check-qobject.c > index 03e9175113..710f9e6b0a 100644 > --- a/tests/check-qobject.c > +++ b/tests/check-qobject.c > @@ -59,6 +59,8 @@ static void do_test_equality(bool expected, int _, ...) > g_assert(qobject_is_equal(args[i], args[j]) == expected); > } > } > + > +g_free(args); > } > > #define check_equal(...) \ Reviewed-by: Markus Armbruster
Re: [Qemu-devel] [PATCH v3 22/50] tests: add some enum members tests
Marc-André Lureau writes:
> Signed-off-by: Marc-André Lureau
> ---
> tests/Makefile.include | 3 +++
> tests/qapi-schema/enum-dict-member-invalid.err | 1 +
> tests/qapi-schema/enum-dict-member-invalid.exit | 1 +
> tests/qapi-schema/enum-dict-member-invalid.json | 2 ++
> tests/qapi-schema/enum-dict-member-invalid.out | 0
> tests/qapi-schema/enum-dict-member-invalid2.err | 1 +
> tests/qapi-schema/enum-dict-member-invalid2.exit | 1 +
> tests/qapi-schema/enum-dict-member-invalid2.json | 2 ++
> tests/qapi-schema/enum-dict-member-invalid2.out | 0
> tests/qapi-schema/enum-if-invalid.err| 1 +
> tests/qapi-schema/enum-if-invalid.exit | 1 +
> tests/qapi-schema/enum-if-invalid.json | 3 +++
> tests/qapi-schema/enum-if-invalid.out| 0
> 13 files changed, 16 insertions(+)
> create mode 100644 tests/qapi-schema/enum-dict-member-invalid.err
> create mode 100644 tests/qapi-schema/enum-dict-member-invalid.exit
> create mode 100644 tests/qapi-schema/enum-dict-member-invalid.json
> create mode 100644 tests/qapi-schema/enum-dict-member-invalid.out
> create mode 100644 tests/qapi-schema/enum-dict-member-invalid2.err
> create mode 100644 tests/qapi-schema/enum-dict-member-invalid2.exit
> create mode 100644 tests/qapi-schema/enum-dict-member-invalid2.json
> create mode 100644 tests/qapi-schema/enum-dict-member-invalid2.out
> create mode 100644 tests/qapi-schema/enum-if-invalid.err
> create mode 100644 tests/qapi-schema/enum-if-invalid.exit
> create mode 100644 tests/qapi-schema/enum-if-invalid.json
> create mode 100644 tests/qapi-schema/enum-if-invalid.out
>
> diff --git a/tests/Makefile.include b/tests/Makefile.include
> index a9f0ddbe01..0aa532f029 100644
> --- a/tests/Makefile.include
> +++ b/tests/Makefile.include
> @@ -443,6 +443,9 @@ qapi-schema += empty.json
> qapi-schema += enum-bad-name.json
> qapi-schema += enum-bad-prefix.json
> qapi-schema += enum-clash-member.json
> +qapi-schema += enum-dict-member-invalid.json
> +qapi-schema += enum-dict-member-invalid2.json
> +qapi-schema += enum-if-invalid.json
> qapi-schema += enum-int-member.json
> qapi-schema += enum-member-case.json
> qapi-schema += enum-missing-data.json
> diff --git a/tests/qapi-schema/enum-dict-member-invalid.err
> b/tests/qapi-schema/enum-dict-member-invalid.err
> new file mode 100644
> index 00..d12cca0df3
> --- /dev/null
> +++ b/tests/qapi-schema/enum-dict-member-invalid.err
> @@ -0,0 +1 @@
> +tests/qapi-schema/enum-dict-member-invalid.json:2: Dictionary member of enum
> 'MyEnum' must have a 'name' key
> diff --git a/tests/qapi-schema/enum-dict-member-invalid.exit
> b/tests/qapi-schema/enum-dict-member-invalid.exit
> new file mode 100644
> index 00..d00491fd7e
> --- /dev/null
> +++ b/tests/qapi-schema/enum-dict-member-invalid.exit
> @@ -0,0 +1 @@
> +1
> diff --git a/tests/qapi-schema/enum-dict-member-invalid.json
> b/tests/qapi-schema/enum-dict-member-invalid.json
> new file mode 100644
> index 00..9cf8406867
> --- /dev/null
> +++ b/tests/qapi-schema/enum-dict-member-invalid.json
> @@ -0,0 +1,2 @@
> +# we reject any enum member that is not a string or a dict with 'name'
> +{ 'enum': 'MyEnum', 'data': [ { 'value': 'str' } ] }
> diff --git a/tests/qapi-schema/enum-dict-member-invalid.out
> b/tests/qapi-schema/enum-dict-member-invalid.out
> new file mode 100644
> index 00..e69de29bb2
> diff --git a/tests/qapi-schema/enum-dict-member-invalid2.err
> b/tests/qapi-schema/enum-dict-member-invalid2.err
> new file mode 100644
> index 00..f7dc1a2b33
> --- /dev/null
> +++ b/tests/qapi-schema/enum-dict-member-invalid2.err
> @@ -0,0 +1 @@
> +tests/qapi-schema/enum-dict-member-invalid2.json:2: Dictionnary has unknown
> keys: bad-key (allowed: name, if)
> diff --git a/tests/qapi-schema/enum-dict-member-invalid2.exit
> b/tests/qapi-schema/enum-dict-member-invalid2.exit
> new file mode 100644
> index 00..d00491fd7e
> --- /dev/null
> +++ b/tests/qapi-schema/enum-dict-member-invalid2.exit
> @@ -0,0 +1 @@
> +1
> diff --git a/tests/qapi-schema/enum-dict-member-invalid2.json
> b/tests/qapi-schema/enum-dict-member-invalid2.json
> new file mode 100644
> index 00..6664c59201
> --- /dev/null
> +++ b/tests/qapi-schema/enum-dict-member-invalid2.json
> @@ -0,0 +1,2 @@
> +# we reject any enum member that is not a string or a dict with 'name'
> +{ 'enum': 'MyEnum', 'data': [ { 'name': 'foo', 'bad-key': 'str' } ] }
> diff --git a/tests/qapi-schema/enum-dict-member-invalid2.out
> b/tests/qapi-schema/enum-dict-member-invalid2.out
> new file mode 100644
> index 00..e69de29bb2
> diff --git a/tests/qapi-schema/enum-if-invalid.err
> b/tests/qapi-schema/enum-if-invalid.err
> new file mode 100644
> index 00..54c3cf887b
> --- /dev/null
> +++ b/tests/qapi-schema/enum-if-invalid.err
> @@ -0,0 +1 @@
> +tests/qapi-schema/enum-if-invalid.json:2: 'if' condition must be a string or
> a list of strings
> diff --
Re: [Qemu-devel] in a device or CPU instance init/realize, can I rely on something having the BQL or equivalent?
On Fri, Dec 08, 2017 at 06:32:03PM +0100, Igor Mammedov wrote: > On Fri, 8 Dec 2017 14:14:22 -0200 > Eduardo Habkost wrote: > > > On Fri, Dec 08, 2017 at 03:50:50PM +0100, Igor Mammedov wrote: > > > On Fri, 8 Dec 2017 13:19:27 + > > > Peter Maydell wrote: > > > > > > > On 8 December 2017 at 13:16, Igor Mammedov wrote: > > > > > > > > > TBH: > > > > > I do not recall why we have x86 max/host cpu types do feature > > > > > loading at realize time instead of at class init like the rest > > > > > of static cpu types. > > > > > > > > class init is too early, IIRC -- it's before KVM has been set up at > > > > all. > > > > > > that shouldn't be an issue as kvm_ppc_register_host_cpu_type() > > > demonstrates > > > (i.e. an additional class init at kvm/tcg init time), > > > > It is possible, but IMO it's not a good idea. We should be able > > to enumerate all CPU types before the accelerator has been > > initialized, so query-cpu-definitions and "-cpu help" will always > > work. > > > > > > > > > > so it might be some compat issue or just legacy approach why it > > > havn't been rewritten to class_init for x86 the way PPC does. > > > But Eduardo probably knows better if there is anything left that > > > prevents using class init there. > > > > It's the opposite: x86 "host" CPU model used to work the same way > > as PPC, but we changed it so all classes are registered at > > type_init()-time. > Is it for libvirt convenience, so that it would be able to cache > all supported cpus regardless of whether they would actually work > or not? I'd say it's for our own convenience, so that things won't break if we make QMP available before machine/accel initialization in the future, or if we reorder the code that handles "-cpu ?" in main(). Moving code around is simpler if we know that all QOM types are registered at the same point, so we can treat them as static data after module_call_init(MODULE_INIT_QOM) runs. -- Eduardo
Re: [Qemu-devel] [PATCH 2/5] lock-guard: add scoped lock implementation
On 08/12/2017 16:30, Stefan Hajnoczi wrote:
> On Fri, Dec 08, 2017 at 11:55:50AM +0100, Paolo Bonzini wrote:
>
> The implementation is somewhat complex. Please structure the header
> file so the public interfaces are clear and documented. Move the
> implementation out of the way and mark it private. That will make it
> easier for someone to figure out "how do I use lock-guard.h".
Right, unfortunately you pretty much have to place it in the header to
guarantee that everything is inlined.
>> diff --git a/include/qemu/lock-guard.h b/include/qemu/lock-guard.h
>> new file mode 100644
>> index 00..e6a83bf9ee
>> --- /dev/null
>> +++ b/include/qemu/lock-guard.h
>> @@ -0,0 +1,103 @@
>> +#ifndef QEMU_LOCK_GUARD_H
>> +#define QEMU_LOCK_GUARD_H 1
>> +
>> +typedef void QemuLockGuardFunc(void *);
>> +typedef struct QemuLockGuard {
>> +QemuLockGuardFunc *p_lock_fn, *p_unlock_fn;
>> +void *lock;
>> +int locked;
>
> bool?
Yes.
>> +#define QEMU_WITH_LOCK(type, name, lock) \
>> +for (QEMU_LOCK_GUARD(type, name, lock);\
>> + qemu_lock_guard_is_taken(&name); \
>> + qemu_lock_guard_unlock(&name))
>
> I don't understand the need for the qemu_lock_guard_is_taken(&name)
> condition, why not do the following?
>
> for (QEMU_LOCK_GUARD(type, name, lock);
>;
>qemu_lock_guard_unlock(&name))
Because that would be an infinite loop. :)
Paolo
> Also, the for loop means that break statements do not work inside
> QEMU_WITH_LOCK() { ... }. This needs to be documented.
Re: [Qemu-devel] [RFC] vhost: check if ring mapping is still valid when building memmap
* Igor Mammedov (imamm...@redhat.com) wrote:
> On Thu, 7 Dec 2017 18:17:51 +
> "Dr. David Alan Gilbert" wrote:
>
> > * Igor Mammedov (imamm...@redhat.com) wrote:
> > > vhost_verify_ring_mappings() were used to verify that
> > > rings are still accessible and related memory hasn't
> > > been moved after flatview is updated.
> > >
> > > It were doing checks by mapping ring's GPA+len and
> > > checking that HVA hasn't changed with new memory map.
> > > To avoid maybe expensive mapping call, we were
> > > identifying address range that changed and were doing
> > > mapping only if ring were in changed range.
> > >
> > > However it's no neccessary to perform ringi's GPA
> > > mapping as we already have it's current HVA and all
> > > we need is to verify that ring's GPA translates to
> > > the same HVA in updated flatview.
> > >
> > > That could be done during time when we are building
> > > vhost memmap where vhost_update_mem_cb() already maps
> > > every RAM MemoryRegionSection to get section HVA. This
> > > fact can be used to check that ring belongs to the same
> > > MemoryRegion in the same place, like this:
> > >
> > > hva_ring_offset = GPA(ring) - GPA(MemoryRegionSection)
> > > ring_hva == HVA(MemoryRegionSection) + hva_ring_offset
> > >
> > > Doing this would allow us to drop ~100LOC of code which
> > > figures out changed memory range and avoid calling not
> > > needed reverse vhost_memory_map(is_write=true) which is
> > > overkill for the task at hand.
> >
> > There are a few things about this I don't understand; however if
> > it's right I agree that it means we can kill off my comparison
> > code.
> >
> >
> > First, can I check what constraints 'verify_ring' needs to check;
> > if I'm understanding the original code it's checking that :
> > a) If a queue falls within a region, that the whole queue can
> >be mapped
> see vhost_verify_ring_part_mapping():
>
> p = vhost_memory_map(dev, part_addr, &l, 1);
>
> if (!p || l != part_size)
> error_out
>
> 1st: (!p) requested part_addr must be mapped
> i.e. be a part of MemorySection in flatview
> AND
> 2nd: (l != part_size) mapped range size must match what we asked for
> i.e. mapped as continuous range so that
> [GPA, GPA + part_size) could directly correspond to [HVA,
> HVA + part_size)
> and that's is possible only withing 1 MemorySection && 1
> MeoryRegion
> if I read address_space_map() correctly
> flatview_translate() -> GPA's MemorySection
> flatview_extend_translation() -> 1:1 GPA->HVA range size
>
> So answer in case of RAM memory region that we are interested in,
> would be:
> queue falls within a MemorySection and whole queue fits in to it
Yes, OK.
> > b) And the start of the queue corresponds to where we thought
> >it used to be (in GPA?)
> that cached at vq->desc queue HVA hasn't changed after flatview
> change
> if (p != part)
>error_out
OK, so it's the HVA that's not changed based on taking the part_addr
which is GPA and checking the map?
> >so that doesn't have any constraint on the ordering of the regions
> >or whether the region is the same size or anything.
> > Also I don't think it would spot if there was a qeueue that had no
> > region associated with it at all.
> >
> > Now, comments on your code below:
> >
> >
> > > Signed-off-by: Igor Mammedov
> > > ---
> > > PS:
> > >should be applied on top of David's series
> > >
> > > ---
> > > include/hw/virtio/vhost.h | 2 -
> > > hw/virtio/vhost.c | 195
> > > ++
> > > 2 files changed, 57 insertions(+), 140 deletions(-)
> > >
> > > diff --git a/include/hw/virtio/vhost.h b/include/hw/virtio/vhost.h
> > > index 467dc77..fc4af5c 100644
> > > --- a/include/hw/virtio/vhost.h
> > > +++ b/include/hw/virtio/vhost.h
> > > @@ -68,8 +68,6 @@ struct vhost_dev {
> > > uint64_t log_size;
> > > Error *migration_blocker;
> > > bool memory_changed;
> > > -hwaddr mem_changed_start_addr;
> > > -hwaddr mem_changed_end_addr;
> > > const VhostOps *vhost_ops;
> > > void *opaque;
> > > struct vhost_log *log;
> > > diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c
> > > index 5b9a7d7..026bac3 100644
> > > --- a/hw/virtio/vhost.c
> > > +++ b/hw/virtio/vhost.c
> > > @@ -296,35 +296,43 @@ static void vhost_memory_unmap(struct vhost_dev
> > > *dev, void *buffer,
> > > }
> > > }
> > >
> > > -static int vhost_verify_ring_part_mapping(struct vhost_dev *dev,
> > > - void *part,
> > > - uint64_t part_addr,
> > > - uint64_t par
Re: [Qemu-devel] [PATCH v3 4/4] ivshmem: Disable irqfd on device reset
On Fri, Dec 8, 2017 at 6:28 PM, Markus Armbruster wrote:
> Ladi Prosek writes:
>
>> On Fri, Dec 8, 2017 at 2:36 PM, Markus Armbruster wrote:
>>> Ladi Prosek writes:
>>>
The effects of ivshmem_enable_irqfd() was not undone on device reset.
This manifested as:
ivshmem_add_kvm_msi_virq: Assertion `!s->msi_vectors[vector].pdev' failed.
when irqfd was enabled before reset and then enabled again after reset,
making
ivshmem_enable_irqfd() run for the second time.
To reproduce, run:
ivshmem-server
and QEMU with:
-device ivshmem-doorbell,chardev=iv
-chardev socket,path=/tmp/ivshmem_socket,id=iv
then install the Windows driver, at the time of writing available at:
https://github.com/virtio-win/kvm-guest-drivers-windows/tree/master/ivshmem
and crash-reboot the guest by inducing a BSOD.
Signed-off-by: Ladi Prosek
---
hw/misc/ivshmem.c | 24 +---
1 file changed, 13 insertions(+), 11 deletions(-)
diff --git a/hw/misc/ivshmem.c b/hw/misc/ivshmem.c
index d1bb246d12..4be0d2627b 100644
--- a/hw/misc/ivshmem.c
+++ b/hw/misc/ivshmem.c
@@ -758,17 +758,6 @@ static void ivshmem_msix_vector_use(IVShmemState *s)
}
}
-static void ivshmem_reset(DeviceState *d)
-{
-IVShmemState *s = IVSHMEM_COMMON(d);
-
-s->intrstatus = 0;
-s->intrmask = 0;
-if (ivshmem_has_feature(s, IVSHMEM_MSI)) {
-ivshmem_msix_vector_use(s);
-}
-}
-
static int ivshmem_setup_interrupts(IVShmemState *s, Error **errp)
{
/* allocate QEMU callback data for receiving interrupts */
@@ -855,6 +844,19 @@ static void ivshmem_disable_irqfd(IVShmemState *s)
}
+static void ivshmem_reset(DeviceState *d)
+{
+IVShmemState *s = IVSHMEM_COMMON(d);
+
+ivshmem_disable_irqfd(s);
+
+s->intrstatus = 0;
+s->intrmask = 0;
+if (ivshmem_has_feature(s, IVSHMEM_MSI)) {
+ivshmem_msix_vector_use(s);
+}
+}
+
static void ivshmem_write_config(PCIDevice *pdev, uint32_t address,
uint32_t val, int len)
{
>>>
>>> Why are you moving ivshmem_reset()? Makes the actual change harder to
>>> see than necessary.
>>
>> ivshmem_disable_irqfd() is declared after ivshmem_reset() in the
>> source file. I generally prefer to order static functions
>> topologically if possible. If you'd prefer adding a forward decl
>> instead (fewer lines touched, easier to bisect?) I can certainly do
>> that. Thanks!
>
> Well, it compiles before your patch, your patch doesn't add any calls,
> so I can't quite see why a forward declaration would be needed.
It adds a call to ivshmem_disable_irqfd().
Re: [Qemu-devel] [PATCH v2 2/6] qapi: add name parameter to nbd-server-add
* Vladimir Sementsov-Ogievskiy (vsement...@virtuozzo.com) wrote:
> Allow user to specify name for new export, to not reuse internal
> node name and to not show it to clients.
>
> This also allows creating several exports per device.
>
> Signed-off-by: Vladimir Sementsov-Ogievskiy
> ---
> qapi/block.json | 9 +++--
> blockdev-nbd.c | 14 +-
> hmp.c | 5 +++--
> 3 files changed, 19 insertions(+), 9 deletions(-)
>
> diff --git a/qapi/block.json b/qapi/block.json
> index f093fa3f27..503d4b287b 100644
> --- a/qapi/block.json
> +++ b/qapi/block.json
> @@ -213,14 +213,19 @@
> #
> # @device: The device name or node name of the node to be exported
> #
> +# @name: Export name. If unspecified @device parameter used as export name.
> +#(Since 2.12)
> +#
> # @writable: Whether clients should be able to write to the device via the
> # NBD connection (default false).
> #
> -# Returns: error if the device is already marked for export.
> +# Returns: error if the server is not running, or export with the same name
> +# already exists.
> #
> # Since: 1.3.0
> ##
> -{ 'command': 'nbd-server-add', 'data': {'device': 'str', '*writable':
> 'bool'} }
> +{ 'command': 'nbd-server-add',
> + 'data': {'device': 'str', '*name': 'str', '*writable': 'bool'} }
>
> ##
> # @nbd-server-stop:
> diff --git a/blockdev-nbd.c b/blockdev-nbd.c
> index 28f551a7b0..46c885aa35 100644
> --- a/blockdev-nbd.c
> +++ b/blockdev-nbd.c
> @@ -158,8 +158,8 @@ void qmp_nbd_server_start(SocketAddressLegacy *addr,
> qapi_free_SocketAddress(addr_flat);
> }
>
> -void qmp_nbd_server_add(const char *device, bool has_writable, bool writable,
> -Error **errp)
> +void qmp_nbd_server_add(const char *device, bool has_name, const char *name,
> +bool has_writable, bool writable, Error **errp)
> {
> BlockDriverState *bs = NULL;
> BlockBackend *on_eject_blk;
> @@ -170,8 +170,12 @@ void qmp_nbd_server_add(const char *device, bool
> has_writable, bool writable,
> return;
> }
>
> -if (nbd_export_find(device)) {
> -error_setg(errp, "NBD server already exporting device '%s'", device);
> +if (!has_name) {
> +name = device;
> +}
> +
> +if (nbd_export_find(name)) {
> +error_setg(errp, "NBD server already has export named '%s'", name);
> return;
> }
>
> @@ -195,7 +199,7 @@ void qmp_nbd_server_add(const char *device, bool
> has_writable, bool writable,
> return;
> }
>
> -nbd_export_set_name(exp, device);
> +nbd_export_set_name(exp, name);
>
> /* The list of named exports has a strong reference to this export now
> and
> * our only way of accessing it is through nbd_export_find(), so we can
> drop
> diff --git a/hmp.c b/hmp.c
> index 35a7041824..0ea9c09b58 100644
> --- a/hmp.c
> +++ b/hmp.c
> @@ -2203,7 +2203,8 @@ void hmp_nbd_server_start(Monitor *mon, const QDict
> *qdict)
> continue;
> }
>
> -qmp_nbd_server_add(info->value->device, true, writable, &local_err);
> +qmp_nbd_server_add(info->value->device, false, NULL,
> + true, writable, &local_err);
>
> if (local_err != NULL) {
> qmp_nbd_server_stop(NULL);
> @@ -2223,7 +2224,7 @@ void hmp_nbd_server_add(Monitor *mon, const QDict
> *qdict)
> bool writable = qdict_get_try_bool(qdict, "writable", false);
> Error *local_err = NULL;
>
> -qmp_nbd_server_add(device, true, writable, &local_err);
> +qmp_nbd_server_add(device, false, NULL, true, writable, &local_err);
I wont insist, but it would be nice if you wired up an optional
parameter on HMP as well.
Dave
> if (local_err != NULL) {
> hmp_handle_error(mon, &local_err);
> --
> 2.11.1
>
--
Dr. David Alan Gilbert / dgilb...@redhat.com / Manchester, UK
Re: [Qemu-devel] in a device or CPU instance init/realize, can I rely on something having the BQL or equivalent?
On Fri, 8 Dec 2017 14:14:22 -0200 Eduardo Habkost wrote: > On Fri, Dec 08, 2017 at 03:50:50PM +0100, Igor Mammedov wrote: > > On Fri, 8 Dec 2017 13:19:27 + > > Peter Maydell wrote: > > > > > On 8 December 2017 at 13:16, Igor Mammedov wrote: > > > > TBH: > > > > I do not recall why we have x86 max/host cpu types do feature > > > > loading at realize time instead of at class init like the rest > > > > of static cpu types. > > > > > > class init is too early, IIRC -- it's before KVM has been set up at all. > > > > that shouldn't be an issue as kvm_ppc_register_host_cpu_type() demonstrates > > (i.e. an additional class init at kvm/tcg init time), > > It is possible, but IMO it's not a good idea. We should be able > to enumerate all CPU types before the accelerator has been > initialized, so query-cpu-definitions and "-cpu help" will always > work. > > > > > > so it might be some compat issue or just legacy approach why it > > havn't been rewritten to class_init for x86 the way PPC does. > > But Eduardo probably knows better if there is anything left that > > prevents using class init there. > > It's the opposite: x86 "host" CPU model used to work the same way > as PPC, but we changed it so all classes are registered at > type_init()-time. Is it for libvirt convenience, so that it would be able to cache all supported cpus regardless of whether they would actually work or not?
Re: [Qemu-devel] [PATCH v3 4/4] ivshmem: Disable irqfd on device reset
Ladi Prosek writes:
> On Fri, Dec 8, 2017 at 2:36 PM, Markus Armbruster wrote:
>> Ladi Prosek writes:
>>
>>> The effects of ivshmem_enable_irqfd() was not undone on device reset.
>>>
>>> This manifested as:
>>> ivshmem_add_kvm_msi_virq: Assertion `!s->msi_vectors[vector].pdev' failed.
>>>
>>> when irqfd was enabled before reset and then enabled again after reset,
>>> making
>>> ivshmem_enable_irqfd() run for the second time.
>>>
>>> To reproduce, run:
>>>
>>> ivshmem-server
>>>
>>> and QEMU with:
>>>
>>> -device ivshmem-doorbell,chardev=iv
>>> -chardev socket,path=/tmp/ivshmem_socket,id=iv
>>>
>>> then install the Windows driver, at the time of writing available at:
>>>
>>> https://github.com/virtio-win/kvm-guest-drivers-windows/tree/master/ivshmem
>>>
>>> and crash-reboot the guest by inducing a BSOD.
>>>
>>> Signed-off-by: Ladi Prosek
>>> ---
>>> hw/misc/ivshmem.c | 24 +---
>>> 1 file changed, 13 insertions(+), 11 deletions(-)
>>>
>>> diff --git a/hw/misc/ivshmem.c b/hw/misc/ivshmem.c
>>> index d1bb246d12..4be0d2627b 100644
>>> --- a/hw/misc/ivshmem.c
>>> +++ b/hw/misc/ivshmem.c
>>> @@ -758,17 +758,6 @@ static void ivshmem_msix_vector_use(IVShmemState *s)
>>> }
>>> }
>>>
>>> -static void ivshmem_reset(DeviceState *d)
>>> -{
>>> -IVShmemState *s = IVSHMEM_COMMON(d);
>>> -
>>> -s->intrstatus = 0;
>>> -s->intrmask = 0;
>>> -if (ivshmem_has_feature(s, IVSHMEM_MSI)) {
>>> -ivshmem_msix_vector_use(s);
>>> -}
>>> -}
>>> -
>>> static int ivshmem_setup_interrupts(IVShmemState *s, Error **errp)
>>> {
>>> /* allocate QEMU callback data for receiving interrupts */
>>> @@ -855,6 +844,19 @@ static void ivshmem_disable_irqfd(IVShmemState *s)
>>>
>>> }
>>>
>>> +static void ivshmem_reset(DeviceState *d)
>>> +{
>>> +IVShmemState *s = IVSHMEM_COMMON(d);
>>> +
>>> +ivshmem_disable_irqfd(s);
>>> +
>>> +s->intrstatus = 0;
>>> +s->intrmask = 0;
>>> +if (ivshmem_has_feature(s, IVSHMEM_MSI)) {
>>> +ivshmem_msix_vector_use(s);
>>> +}
>>> +}
>>> +
>>> static void ivshmem_write_config(PCIDevice *pdev, uint32_t address,
>>> uint32_t val, int len)
>>> {
>>
>> Why are you moving ivshmem_reset()? Makes the actual change harder to
>> see than necessary.
>
> ivshmem_disable_irqfd() is declared after ivshmem_reset() in the
> source file. I generally prefer to order static functions
> topologically if possible. If you'd prefer adding a forward decl
> instead (fewer lines touched, easier to bisect?) I can certainly do
> that. Thanks!
Well, it compiles before your patch, your patch doesn't add any calls,
so I can't quite see why a forward declaration would be needed.
Re: [Qemu-devel] [Qemu-block] [PATCH] blockdev-backup: enable non-root nodes for backup
On 12/08/2017 09:30 AM, Max Reitz wrote: > On 2017-12-05 01:48, John Snow wrote: >> >> >> On 12/04/2017 05:21 PM, Max Reitz wrote: >>> On 2017-12-04 23:15, John Snow wrote: On 12/01/2017 02:41 PM, Max Reitz wrote: > ((By the way, I don't suppose that's how it should work... But I don't > suppose that we want propagation of dirtying towards the BDS roots, do > we? :-/)) I have never really satisfactorily explained to myself what bitmaps on intermediate notes truly represent or mean. The simple case is "This layer itself serviced a write request." If that information is not necessarily meaningful, I'm not sure that's a problem except in configuration. ...Now, if you wanted to talk about bitmaps that associate with a Backend instead of a Node... >>> >>> But it's not about bitmaps on intermediate nodes, quite the opposite. >>> It's about bitmaps on roots but write requests happening on intermediate >>> nodes. >>> >> >> Oh, I see what you're saying. It magically doesn't really change my >> opinion, by coincidence! >> >>> Say you have a node I and two filter nodes A and B using it (and they >>> are OK with shared writers). There is a dirty bitmap on A. >>> >>> Now when a write request goes through B, I will obviously have changed, >>> and because A and B are filters, so will A. But the dirty bitmap on A >>> will still be clean. >>> >>> My example was that when you run a mirror over A, you won't see dirtying >>> from B. So you can't e.g. add a throttle driver between a mirror job >>> and the node you want to mirror, because the dirty bitmap on the >>> throttle driver will not be affected by accesses to the actual node. >>> >>> Max >>> >> >> Well, in this case I would say that a root BDS is not really any >> different from an intermediate one and can't really know what's going on >> in the world outside. >> >> At least, I think that's how we model it right now -- we pretend that we >> can record the activity of an entire drive graph by putting the bitmap >> on the root-most node we can get a hold of and assuming that all writes >> are going to go through us. > > Well, yeah, I know we do. But I consider this counter-intuitive and if > something is counter-intuitive it's often a bug. > >> Clearly this is increasingly false the more we modularise the block graph. >> >> >> *uhm* >> >> >> I would say that a bitmap attached to a BlockBackend should behave in >> the way you say: writes to any children should change the bitmap here. >> >> bitmaps attached to nodes shouldn't worry about such things. > > Do we have bitmaps attached to BlockBackends? I sure hope not. > > We should not have any interface that requires the use of BlockBackends > by now. If we do, that's something that has to be fixed. > > Max > I'm not sure what the right paradigm is anymore, then. A node is just a node, but something has to represent the "drive" as far as the device model sees it. I thought that *was* the BlockBackend, but is it not? --js
Re: [Qemu-devel] [PATCH 2/2] block/iscsi: only report an iSCSI Failure if we don't handle it gracefully
On 12/08/2017 10:14 AM, Peter Lieven wrote: > Am 08.12.2017 um 16:11 schrieb Eric Blake: >> On 12/08/2017 05:51 AM, Peter Lieven wrote: >>> we currently report an "iSCSI Failure" in iscsi_co_generic_cb if the task >>> hasn't completed with SCSI_STATUS_GOOD. However, we expect a failure in >>> some cases and handle it gracefully. This is the case for misaligned UNMAPs >> Is the block layer still capable of producing a misaligned UNMAP? If >> so, that's probably a bug in the block layer for not honoring the block >> limit geometries. > > In theory there should be none. I think we can drop this code. Or, better yet, replace the check with an assert. -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org signature.asc Description: OpenPGP digital signature
[Qemu-devel] [PATCH] sparc: Make sure we mmap at SHMLBA alignment
SPARC Linux has an oddity that it insists that mmap() of MAP_FIXED memory must be at an alignment defined by SHMLBA, which is more aligned than the page size (typically, SHMLBA alignment is to 16K, and pages are 8K). This is a relic of ancient hardware that had cache aliasing constraints, but even on modern hardware the kernel still insists on the alignment. To ensure that we get mmap() alignment sufficient to make the kernel happy, change QEMU_VMALLOC_ALIGN, qemu_fd_getpagesize() and qemu_mempath_getpagesize() to use the maximum of getpagesize() and SHMLBA. In particular, this allows 'make check' to pass on Sparc: we were previously failing the ivshmem tests. Signed-off-by: Peter Maydell --- include/qemu/osdep.h | 3 +++ util/mmap-alloc.c| 8 2 files changed, 11 insertions(+) diff --git a/include/qemu/osdep.h b/include/qemu/osdep.h index e8568a0..adb3758 100644 --- a/include/qemu/osdep.h +++ b/include/qemu/osdep.h @@ -365,6 +365,9 @@ void qemu_anon_ram_free(void *ptr, size_t size); #elif defined(__linux__) && defined(__s390x__) /* Use 1 MiB (segment size) alignment so gmap can be used by KVM. */ # define QEMU_VMALLOC_ALIGN (256 * 4096) +#elif defined(__linux__) && defined(__sparc__) +#include +# define QEMU_VMALLOC_ALIGN MAX(getpagesize(), SHMLBA) #else # define QEMU_VMALLOC_ALIGN getpagesize() #endif diff --git a/util/mmap-alloc.c b/util/mmap-alloc.c index 3ec029a..2fd8cbc 100644 --- a/util/mmap-alloc.c +++ b/util/mmap-alloc.c @@ -35,6 +35,10 @@ size_t qemu_fd_getpagesize(int fd) return fs.f_bsize; } } +#ifdef __sparc__ +/* SPARC Linux needs greater alignment than the pagesize */ +return QEMU_VMALLOC_ALIGN; +#endif #endif return getpagesize(); @@ -60,6 +64,10 @@ size_t qemu_mempath_getpagesize(const char *mem_path) /* It's hugepage, return the huge page size */ return fs.f_bsize; } +#ifdef __sparc__ +/* SPARC Linux needs greater alignment than the pagesize */ +return QEMU_VMALLOC_ALIGN; +#endif #endif return getpagesize(); -- 2.7.4
[Qemu-devel] [PATCH v4 for-2-12] s390x: change the QEMU cpu model to a stripped down z12
We are good enough to boot upstream Linux kernels / Fedora 26/27. That
should be sufficient for now.
As the QEMU CPU model is migration safe, let's add compatibility code.
Generate the feature list to reduce the chance of messing things up in the
future.
Signed-off-by: David Hildenbrand
---
hw/s390x/s390-virtio-ccw.c | 8
target/s390x/cpu.h | 3 ++
target/s390x/cpu_models.c | 103 +++-
target/s390x/cpu_models.h | 1 +
target/s390x/gen-features.c | 87 +
5 files changed, 143 insertions(+), 59 deletions(-)
diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
index a23b8aec9f..b76d87bb5d 100644
--- a/hw/s390x/s390-virtio-ccw.c
+++ b/hw/s390x/s390-virtio-ccw.c
@@ -721,6 +721,10 @@ bool css_migration_enabled(void)
static void ccw_machine_2_12_instance_options(MachineState *machine)
{
+static const S390FeatInit qemu_cpu_feat = { S390_FEAT_LIST_QEMU_V2_12 };
+
+/* with 2.12 we emulated a stripped down zEC12 (GA 2) */
+s390_set_qemu_cpu_model(0x2827, 12, 2, qemu_cpu_feat);
}
static void ccw_machine_2_12_class_options(MachineClass *mc)
@@ -730,7 +734,11 @@ DEFINE_CCW_MACHINE(2_12, "2.12", true);
static void ccw_machine_2_11_instance_options(MachineState *machine)
{
+static const S390FeatInit qemu_cpu_feat = { S390_FEAT_LIST_QEMU_V2_11 };
ccw_machine_2_12_instance_options(machine);
+
+/* before 2.12 we emulated the very first z900 */
+s390_set_qemu_cpu_model(0x2064, 7, 1, qemu_cpu_feat);
}
static void ccw_machine_2_11_class_options(MachineClass *mc)
diff --git a/target/s390x/cpu.h b/target/s390x/cpu.h
index f9d4d62c48..1a8b6b9ae9 100644
--- a/target/s390x/cpu.h
+++ b/target/s390x/cpu.h
@@ -722,6 +722,9 @@ static inline unsigned int s390_cpu_set_state(uint8_t
cpu_state, S390CPU *cpu)
/* cpu_models.c */
void s390_cpu_list(FILE *f, fprintf_function cpu_fprintf);
#define cpu_list s390_cpu_list
+void s390_set_qemu_cpu_model(uint16_t type, uint8_t gen, uint8_t ec_ga,
+ const S390FeatInit feat_init);
+
/* helper.c */
#define cpu_init(cpu_model) cpu_generic_init(TYPE_S390_CPU, cpu_model)
diff --git a/target/s390x/cpu_models.c b/target/s390x/cpu_models.c
index edac7fdecf..7404ef52c6 100644
--- a/target/s390x/cpu_models.c
+++ b/target/s390x/cpu_models.c
@@ -15,7 +15,6 @@
#include "internal.h"
#include "kvm_s390x.h"
#include "sysemu/kvm.h"
-#include "gen-features.h"
#include "qapi/error.h"
#include "qapi/visitor.h"
#include "qemu/error-report.h"
@@ -81,6 +80,12 @@ static S390CPUDef s390_cpu_defs[] = {
CPUDEF_INIT(0x3906, 14, 1, 47, 0x0800U, "z14", "IBM z14 GA1"),
};
+#define QEMU_MAX_CPU_TYPE 0x2827
+#define QEMU_MAX_CPU_GEN 12
+#define QEMU_MAX_CPU_EC_GA 2
+static const S390FeatInit qemu_max_cpu_feat_init = { S390_FEAT_LIST_QEMU_MAX };
+static S390FeatBitmap qemu_max_cpu_feat;
+
/* features part of a base model but not relevant for finding a base model */
S390FeatBitmap ignored_base_feat;
@@ -812,51 +817,6 @@ static void check_compatibility(const S390CPUModel
*max_model,
"available in the configuration: ");
}
-/**
- * The base TCG CPU model "qemu" is based on the z900. However, we already
- * can also emulate some additional features of later CPU generations, so
- * we add these additional feature bits here.
- */
-static void add_qemu_cpu_model_features(S390FeatBitmap fbm)
-{
-static const int feats[] = {
-S390_FEAT_DAT_ENH,
-S390_FEAT_IDTE_SEGMENT,
-S390_FEAT_STFLE,
-S390_FEAT_SENSE_RUNNING_STATUS,
-S390_FEAT_EXTENDED_TRANSLATION_2,
-S390_FEAT_MSA,
-S390_FEAT_LONG_DISPLACEMENT,
-S390_FEAT_LONG_DISPLACEMENT_FAST,
-S390_FEAT_EXTENDED_IMMEDIATE,
-S390_FEAT_EXTENDED_TRANSLATION_3,
-S390_FEAT_ETF2_ENH,
-S390_FEAT_STORE_CLOCK_FAST,
-S390_FEAT_MOVE_WITH_OPTIONAL_SPEC,
-S390_FEAT_ETF3_ENH,
-S390_FEAT_EXTRACT_CPU_TIME,
-S390_FEAT_COMPARE_AND_SWAP_AND_STORE,
-S390_FEAT_COMPARE_AND_SWAP_AND_STORE_2,
-S390_FEAT_GENERAL_INSTRUCTIONS_EXT,
-S390_FEAT_EXECUTE_EXT,
-S390_FEAT_SET_PROGRAM_PARAMETERS,
-S390_FEAT_FLOATING_POINT_SUPPPORT_ENH,
-S390_FEAT_STFLE_45,
-S390_FEAT_STFLE_49,
-S390_FEAT_LOCAL_TLB_CLEARING,
-S390_FEAT_INTERLOCKED_ACCESS_2,
-S390_FEAT_STFLE_53,
-S390_FEAT_MSA_EXT_5,
-S390_FEAT_MSA_EXT_3,
-S390_FEAT_MSA_EXT_4,
-};
-int i;
-
-for (i = 0; i < ARRAY_SIZE(feats); i++) {
-set_bit(feats[i], fbm);
-}
-}
-
static S390CPUModel *get_max_cpu_model(Error **errp)
{
static S390CPUModel max_model;
@@ -869,12 +829,10 @@ static S390CPUModel *get_max_cpu_model(Error **errp)
if (kvm_enabled()) {
kvm_s390_get_host_cpu_model(&max_model, errp);
} else {
-/* TCG emulates a z900 (with some optional additional featu
Re: [Qemu-devel] [PATCH v3 for-2.12 14/14] s390x: change the QEMU cpu model to a stripped down z12
On 08.12.2017 17:34, Daniel P. Berrange wrote:
> On Fri, Dec 08, 2017 at 05:29:36PM +0100, David Hildenbrand wrote:
>> On 08.12.2017 17:26, Cornelia Huck wrote:
>>> On Fri, 8 Dec 2017 17:02:07 +0100
>>> David Hildenbrand wrote:
>>>
We are good enough to boot upstream Linux kernels / Fedora 26/27. That
should be sufficient for now.
As the QEMU CPU model is migration safe, let's add compatibility code.
Generate the feature list to reduce the chance of messing things up in the
future.
Signed-off-by: David Hildenbrand
---
hw/s390x/s390-virtio-ccw.c | 8
target/s390x/cpu.h | 3 ++
target/s390x/cpu_models.c | 100
++--
target/s390x/cpu_models.h | 1 +
target/s390x/gen-features.c | 87 ++
5 files changed, 140 insertions(+), 59 deletions(-)
>>>
>>> Unfortunately, this patch makes mingw unhappy (x86_64-w64-mingw32 on my
>>> F26 laptop):
>>>
>>> In file included from /home/cohuck/git/qemu/target/s390x/cpu_models.h:17:0,
>>> from /home/cohuck/git/qemu/target/s390x/cpu.h:28,
>>> from /home/cohuck/git/qemu/hw/s390x/s390-virtio-ccw.c:16:
>>> /home/cohuck/git/qemu/hw/s390x/s390-virtio-ccw.c: In function
>>> 'ccw_machine_2_12_instance_options':
>>> ./gen-features.h:96:35: error: large integer implicitly truncated to
>>> unsigned type [-Werror=overflow]
>>> #define S390_FEAT_LIST_QEMU_V2_12
>>> 0x3000e918fd6de14fULL,0x000ff000ULL,0xULL,0xULL
>>>^
>>> /home/cohuck/git/qemu/hw/s390x/s390-virtio-ccw.c:728:51: note: in expansion
>>> of macro 'S390_FEAT_LIST_QEMU_V2_12'
>>> static const S390FeatBitmap qemu_cpu_feat = {
>>> S390_FEAT_LIST_QEMU_V2_12 };
>>>^
>>
>> Huh? we have ULL added to all constants, this should not get reported.
>
> Isn't the problem here the S390FeatBitmap type which is a 'unsigned long',
> so not guaranteed to be 64-bits on 32-bit hosts ?
Right, I remember why I needed that ugly hack in cpu_model.c, where we
initialize the bitmap from an array (e.g. base_init). Thanks for the hint.
>
> Regards,
> Daniel
>
--
Thanks,
David / dhildenb
Re: [Qemu-devel] [PATCH v3 for-2.12 14/14] s390x: change the QEMU cpu model to a stripped down z12
On Fri, Dec 08, 2017 at 05:29:36PM +0100, David Hildenbrand wrote:
> On 08.12.2017 17:26, Cornelia Huck wrote:
> > On Fri, 8 Dec 2017 17:02:07 +0100
> > David Hildenbrand wrote:
> >
> >> We are good enough to boot upstream Linux kernels / Fedora 26/27. That
> >> should be sufficient for now.
> >>
> >> As the QEMU CPU model is migration safe, let's add compatibility code.
> >> Generate the feature list to reduce the chance of messing things up in the
> >> future.
> >>
> >> Signed-off-by: David Hildenbrand
> >> ---
> >> hw/s390x/s390-virtio-ccw.c | 8
> >> target/s390x/cpu.h | 3 ++
> >> target/s390x/cpu_models.c | 100
> >> ++--
> >> target/s390x/cpu_models.h | 1 +
> >> target/s390x/gen-features.c | 87 ++
> >> 5 files changed, 140 insertions(+), 59 deletions(-)
> >
> > Unfortunately, this patch makes mingw unhappy (x86_64-w64-mingw32 on my
> > F26 laptop):
> >
> > In file included from /home/cohuck/git/qemu/target/s390x/cpu_models.h:17:0,
> > from /home/cohuck/git/qemu/target/s390x/cpu.h:28,
> > from /home/cohuck/git/qemu/hw/s390x/s390-virtio-ccw.c:16:
> > /home/cohuck/git/qemu/hw/s390x/s390-virtio-ccw.c: In function
> > 'ccw_machine_2_12_instance_options':
> > ./gen-features.h:96:35: error: large integer implicitly truncated to
> > unsigned type [-Werror=overflow]
> > #define S390_FEAT_LIST_QEMU_V2_12
> > 0x3000e918fd6de14fULL,0x000ff000ULL,0xULL,0xULL
> >^
> > /home/cohuck/git/qemu/hw/s390x/s390-virtio-ccw.c:728:51: note: in expansion
> > of macro 'S390_FEAT_LIST_QEMU_V2_12'
> > static const S390FeatBitmap qemu_cpu_feat = {
> > S390_FEAT_LIST_QEMU_V2_12 };
> >^
>
> Huh? we have ULL added to all constants, this should not get reported.
Isn't the problem here the S390FeatBitmap type which is a 'unsigned long',
so not guaranteed to be 64-bits on 32-bit hosts ?
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 v2 0/6] block: Fix BlockDriver callbacks in bdrv_drain_all_begin()
Am 06.12.2017 um 11:53 hat Kevin Wolf geschrieben: > I was looking into the drain functions in order to develop them a bit in > the direction that Fam suggested, to unify the code between bdrv_drain() > and bdrv_drain_all() a bit more, and maybe to find a place to take > coroutine locks for graph changes. > > The first thing I found is a bug in bdrv_drain_all(), so I'm already > sending this part before I have made much progress with my actual plan. > > v2: > - Added patches 5 and 6 [Paolo] > - Fixed commit message of patch 1 [Eric] Applied to block-next. Kevin
Re: [Qemu-devel] [PATCH v3 for-2.12 14/14] s390x: change the QEMU cpu model to a stripped down z12
On 08.12.2017 17:26, Cornelia Huck wrote:
> On Fri, 8 Dec 2017 17:02:07 +0100
> David Hildenbrand wrote:
>
>> We are good enough to boot upstream Linux kernels / Fedora 26/27. That
>> should be sufficient for now.
>>
>> As the QEMU CPU model is migration safe, let's add compatibility code.
>> Generate the feature list to reduce the chance of messing things up in the
>> future.
>>
>> Signed-off-by: David Hildenbrand
>> ---
>> hw/s390x/s390-virtio-ccw.c | 8
>> target/s390x/cpu.h | 3 ++
>> target/s390x/cpu_models.c | 100
>> ++--
>> target/s390x/cpu_models.h | 1 +
>> target/s390x/gen-features.c | 87 ++
>> 5 files changed, 140 insertions(+), 59 deletions(-)
>
> Unfortunately, this patch makes mingw unhappy (x86_64-w64-mingw32 on my
> F26 laptop):
>
> In file included from /home/cohuck/git/qemu/target/s390x/cpu_models.h:17:0,
> from /home/cohuck/git/qemu/target/s390x/cpu.h:28,
> from /home/cohuck/git/qemu/hw/s390x/s390-virtio-ccw.c:16:
> /home/cohuck/git/qemu/hw/s390x/s390-virtio-ccw.c: In function
> 'ccw_machine_2_12_instance_options':
> ./gen-features.h:96:35: error: large integer implicitly truncated to unsigned
> type [-Werror=overflow]
> #define S390_FEAT_LIST_QEMU_V2_12
> 0x3000e918fd6de14fULL,0x000ff000ULL,0xULL,0xULL
>^
> /home/cohuck/git/qemu/hw/s390x/s390-virtio-ccw.c:728:51: note: in expansion
> of macro 'S390_FEAT_LIST_QEMU_V2_12'
> static const S390FeatBitmap qemu_cpu_feat = { S390_FEAT_LIST_QEMU_V2_12
> };
>^
Huh? we have ULL added to all constants, this should not get reported.
--
Thanks,
David / dhildenb
Re: [Qemu-devel] [PATCH v3 for-2.12 14/14] s390x: change the QEMU cpu model to a stripped down z12
On Fri, 8 Dec 2017 17:02:07 +0100
David Hildenbrand wrote:
> We are good enough to boot upstream Linux kernels / Fedora 26/27. That
> should be sufficient for now.
>
> As the QEMU CPU model is migration safe, let's add compatibility code.
> Generate the feature list to reduce the chance of messing things up in the
> future.
>
> Signed-off-by: David Hildenbrand
> ---
> hw/s390x/s390-virtio-ccw.c | 8
> target/s390x/cpu.h | 3 ++
> target/s390x/cpu_models.c | 100
> ++--
> target/s390x/cpu_models.h | 1 +
> target/s390x/gen-features.c | 87 ++
> 5 files changed, 140 insertions(+), 59 deletions(-)
Unfortunately, this patch makes mingw unhappy (x86_64-w64-mingw32 on my
F26 laptop):
In file included from /home/cohuck/git/qemu/target/s390x/cpu_models.h:17:0,
from /home/cohuck/git/qemu/target/s390x/cpu.h:28,
from /home/cohuck/git/qemu/hw/s390x/s390-virtio-ccw.c:16:
/home/cohuck/git/qemu/hw/s390x/s390-virtio-ccw.c: In function
'ccw_machine_2_12_instance_options':
./gen-features.h:96:35: error: large integer implicitly truncated to unsigned
type [-Werror=overflow]
#define S390_FEAT_LIST_QEMU_V2_12
0x3000e918fd6de14fULL,0x000ff000ULL,0xULL,0xULL
^
/home/cohuck/git/qemu/hw/s390x/s390-virtio-ccw.c:728:51: note: in expansion of
macro 'S390_FEAT_LIST_QEMU_V2_12'
static const S390FeatBitmap qemu_cpu_feat = { S390_FEAT_LIST_QEMU_V2_12 };
^
>
> diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
> index a23b8aec9f..9666fca04f 100644
> --- a/hw/s390x/s390-virtio-ccw.c
> +++ b/hw/s390x/s390-virtio-ccw.c
> @@ -721,6 +721,10 @@ bool css_migration_enabled(void)
>
> static void ccw_machine_2_12_instance_options(MachineState *machine)
> {
> +static const S390FeatBitmap qemu_cpu_feat = { S390_FEAT_LIST_QEMU_V2_12
> };
> +
> +/* with 2.12 we emulated a stripped down zEC12 (GA 2) */
> +s390_set_qemu_cpu_model(0x2827, 12, 2, qemu_cpu_feat);
> }
>
> static void ccw_machine_2_12_class_options(MachineClass *mc)
> @@ -730,7 +734,11 @@ DEFINE_CCW_MACHINE(2_12, "2.12", true);
>
> static void ccw_machine_2_11_instance_options(MachineState *machine)
> {
> +static const S390FeatBitmap qemu_cpu_feat = { S390_FEAT_LIST_QEMU_V2_11
> };
> ccw_machine_2_12_instance_options(machine);
> +
> +/* before 2.12 we emulated the very first z900 */
> +s390_set_qemu_cpu_model(0x2064, 7, 1, qemu_cpu_feat);
> }
>
> static void ccw_machine_2_11_class_options(MachineClass *mc)
> diff --git a/target/s390x/cpu.h b/target/s390x/cpu.h
> index f9d4d62c48..6a91739ece 100644
> --- a/target/s390x/cpu.h
> +++ b/target/s390x/cpu.h
> @@ -722,6 +722,9 @@ static inline unsigned int s390_cpu_set_state(uint8_t
> cpu_state, S390CPU *cpu)
> /* cpu_models.c */
> void s390_cpu_list(FILE *f, fprintf_function cpu_fprintf);
> #define cpu_list s390_cpu_list
> +void s390_set_qemu_cpu_model(uint16_t type, uint8_t gen, uint8_t ec_ga,
> + const S390FeatBitmap features);
> +
>
> /* helper.c */
> #define cpu_init(cpu_model) cpu_generic_init(TYPE_S390_CPU, cpu_model)
> diff --git a/target/s390x/cpu_models.c b/target/s390x/cpu_models.c
> index edac7fdecf..f0577f8155 100644
> --- a/target/s390x/cpu_models.c
> +++ b/target/s390x/cpu_models.c
> @@ -15,7 +15,6 @@
> #include "internal.h"
> #include "kvm_s390x.h"
> #include "sysemu/kvm.h"
> -#include "gen-features.h"
> #include "qapi/error.h"
> #include "qapi/visitor.h"
> #include "qemu/error-report.h"
> @@ -81,6 +80,11 @@ static S390CPUDef s390_cpu_defs[] = {
> CPUDEF_INIT(0x3906, 14, 1, 47, 0x0800U, "z14", "IBM z14 GA1"),
> };
>
> +#define QEMU_MAX_CPU_TYPE 0x2827
> +#define QEMU_MAX_CPU_GEN 12
> +#define QEMU_MAX_CPU_EC_GA 2
> +static const S390FeatBitmap qemu_max_cpu_feat = { S390_FEAT_LIST_QEMU_MAX };
> +
> /* features part of a base model but not relevant for finding a base model */
> S390FeatBitmap ignored_base_feat;
>
> @@ -812,51 +816,6 @@ static void check_compatibility(const S390CPUModel
> *max_model,
>"available in the configuration: ");
> }
>
> -/**
> - * The base TCG CPU model "qemu" is based on the z900. However, we already
> - * can also emulate some additional features of later CPU generations, so
> - * we add these additional feature bits here.
> - */
> -static void add_qemu_cpu_model_features(S390FeatBitmap fbm)
> -{
> -static const int feats[] = {
> -S390_FEAT_DAT_ENH,
> -S390_FEAT_IDTE_SEGMENT,
> -S390_FEAT_STFLE,
> -S390_FEAT_SENSE_RUNNING_STATUS,
> -S390_FEAT_EXTENDED_TRANSLATION_2,
> -S390_FEAT_MSA,
> -S390_FEAT_LONG_DISPLACEMENT,
> -S390_FEAT_LONG_DISPLACEMENT_FAST,
> -S390_FEAT_EXTENDED_IMMEDIATE,
> -
Re: [Qemu-devel] [PATCH v3 4/4] ivshmem: Disable irqfd on device reset
On 12/08/2017 07:44 AM, Ladi Prosek wrote:
>>> static void ivshmem_write_config(PCIDevice *pdev, uint32_t address,
>>> uint32_t val, int len)
>>> {
>>
>> Why are you moving ivshmem_reset()? Makes the actual change harder to
>> see than necessary.
>
> ivshmem_disable_irqfd() is declared after ivshmem_reset() in the
> source file. I generally prefer to order static functions
> topologically if possible. If you'd prefer adding a forward decl
> instead (fewer lines touched, easier to bisect?) I can certainly do
> that. Thanks!
That, or split it into two patches (one doing just the code motion, the
other making the semantic change).
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3266
Virtualization: qemu.org | libvirt.org
signature.asc
Description: OpenPGP digital signature
Re: [Qemu-devel] [PATCH v2] qemu-io: fix EOF Ctrl-D handling in qemu-io readline code
On Fri, Dec 08, 2017 at 10:21:35AM -0600, Eric Blake wrote:
> On 12/08/2017 07:34 AM, Daniel P. Berrange wrote:
> > qemu-io puts the TTY into non-canonical mode, which means no EOF processing
> > is
> > done and thus getchar() will never return the EOF constant. Instead we have
> > to
> > query the TTY attributes to determine the configured EOF character (usually
> > Ctrl-D / 0x4), and then explicitly check for that value. This fixes the
> > regression that prevented Ctrl-D from triggering an exit of qemu-io that has
> > existed since readline was first added in
> >
> > commit 0cf17e181798063c3824c8200ba46f25f54faa1a
> > Author: Stefan Hajnoczi
> > Date: Thu Nov 14 11:54:17 2013 +0100
> >
> > qemu-io: use readline.c
> >
> > Signed-off-by: Daniel P. Berrange
> > ---
> > Changed in v2:
>
> > +++ b/qemu-io.c
> > @@ -10,6 +10,9 @@
> > #include "qemu/osdep.h"
> > #include
> > #include
> > +#ifndef _WIN32
> > +#include
> > +#endif
>
> Wouldn't a configure probe for the existence of be more
> reliable than just hard-coding the list of platforms where it is
> currently not found?
>
> >
> > #include "qapi/error.h"
> > #include "qemu-io.h"
> > @@ -41,6 +44,26 @@ static bool imageOpts;
> >
> > static ReadLineState *readline_state;
> >
> > +static int ttyEOF;
> > +
> > +static int get_eof_char(void)
> > +{
> > +#ifdef _WIN32
>
> in which case this also should be #if HAVE_TERMIOS_H
>
> But I guess unless someone complains, all other platforms that we care
> about have termios.h, so a weak:
FYI, vl.c already includes termios.h, with merely #ifndef _WIN32
protection, so that shows this is sufficient for our immediate
needs.
> Reviewed-by: Eric Blake
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 v2] qemu-io: fix EOF Ctrl-D handling in qemu-io readline code
On 12/08/2017 07:34 AM, Daniel P. Berrange wrote:
> qemu-io puts the TTY into non-canonical mode, which means no EOF processing is
> done and thus getchar() will never return the EOF constant. Instead we have to
> query the TTY attributes to determine the configured EOF character (usually
> Ctrl-D / 0x4), and then explicitly check for that value. This fixes the
> regression that prevented Ctrl-D from triggering an exit of qemu-io that has
> existed since readline was first added in
>
> commit 0cf17e181798063c3824c8200ba46f25f54faa1a
> Author: Stefan Hajnoczi
> Date: Thu Nov 14 11:54:17 2013 +0100
>
> qemu-io: use readline.c
>
> Signed-off-by: Daniel P. Berrange
> ---
> Changed in v2:
> +++ b/qemu-io.c
> @@ -10,6 +10,9 @@
> #include "qemu/osdep.h"
> #include
> #include
> +#ifndef _WIN32
> +#include
> +#endif
Wouldn't a configure probe for the existence of be more
reliable than just hard-coding the list of platforms where it is
currently not found?
>
> #include "qapi/error.h"
> #include "qemu-io.h"
> @@ -41,6 +44,26 @@ static bool imageOpts;
>
> static ReadLineState *readline_state;
>
> +static int ttyEOF;
> +
> +static int get_eof_char(void)
> +{
> +#ifdef _WIN32
in which case this also should be #if HAVE_TERMIOS_H
But I guess unless someone complains, all other platforms that we care
about have termios.h, so a weak:
Reviewed-by: Eric Blake
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3266
Virtualization: qemu.org | libvirt.org
signature.asc
Description: OpenPGP digital signature
Re: [Qemu-devel] [virtio-dev] [PATCH v3 0/7] Vhost-pci for inter-VM communication
On Fri, Dec 8, 2017 at 2:27 PM, Michael S. Tsirkin wrote: > On Fri, Dec 08, 2017 at 06:08:05AM +, Stefan Hajnoczi wrote: >> On Thu, Dec 7, 2017 at 11:54 PM, Michael S. Tsirkin wrote: >> > On Thu, Dec 07, 2017 at 06:28:19PM +, Stefan Hajnoczi wrote: >> >> On Thu, Dec 7, 2017 at 5:38 PM, Michael S. Tsirkin >> >> wrote: >> >> > On Thu, Dec 07, 2017 at 05:29:14PM +, Stefan Hajnoczi wrote: >> >> >> On Thu, Dec 7, 2017 at 4:47 PM, Michael S. Tsirkin >> >> >> wrote: >> >> >> > On Thu, Dec 07, 2017 at 04:29:45PM +, Stefan Hajnoczi wrote: >> >> >> >> On Thu, Dec 7, 2017 at 2:02 PM, Michael S. Tsirkin >> >> >> >> wrote: >> >> >> >> > On Thu, Dec 07, 2017 at 01:08:04PM +, Stefan Hajnoczi wrote: >> >> >> >> > Besides, this means implementing iotlb in both qemu and guest. >> >> >> >> It's free in the guest, the libvhost-user stack already has it. >> > >> > That library is designed to work with a unix domain socket >> > though. We'll need extra kernel code to make a device >> > pretend it's a socket. >> >> A kernel vhost-pci driver isn't necessary because I don't think there >> are in-kernel users. >> >> A vfio vhost-pci backend can go alongside the UNIX domain socket >> backend that exists today in libvhost-user. >> >> If we want to expose kernel vhost devices via vhost-pci then a >> libvhost-user program can translate the vhost-user protocol into >> kernel ioctls. For example: >> $ vhost-pci-proxy --vhost-pci-addr 00:04.0 --vhost-fd 3 3<>/dev/vhost-net >> >> The vhost-pci-proxy implements the vhost-user protocol callbacks and >> submits ioctls on the vhost kernel device fd. I haven't compared the >> kernel ioctl interface vs the vhost-user protocol to see if everything >> maps cleanly though. >> >> Stefan > > I don't really like this, it's yet another package to install, yet > another process to complicate debugging and yet another service that can > go down. > > Maybe vsock can do the trick though? Not sure what you have in mind. An in-kernel vhost-pci driver is possible too. The neatest integration would be alongside drivers/vhost/vhost.c so that existing net, scsi, vsock vhost drivers can work with vhost-pci. Userspace still needs to configure the devices and associate them with a vhost-pci instance (e.g. which vhost-pci device should be a vhost_scsi target and the SCSI target configuration). But I think this approach is more work than the vhost-pci-proxy program I've described. Stefan
Re: [Qemu-devel] [PATCH] docs: update information for TLS certificate management
On 12/08/2017 05:58 AM, Daniel P. Berrange wrote:
> The current docs for TLS assume only VNC is using TLS. Some of the information
> is also outdated (ie lacking subject alt name info for certs). Rewrite it to
> more accurately reflect the current situation.
>
> Signed-off-by: Daniel P. Berrange
> ---
> qemu-doc.texi | 368
> +++---
> 1 file changed, 275 insertions(+), 93 deletions(-)
>
> +@subsection Configuring SASL mechanisms
> +
> +The following documentation assumes use of the Cyrus SASL implementation on a
> +Linux host, but the principals should apply to any other SASL impl. When SASL
s/impl/implementation/
> +is enabled, the mechanism configuration will be loaded from system default
> +SASL service config /etc/sasl2/qemu.conf. If running QEMU as an
> +unprivileged user, an environment variable SASL_CONF_PATH can be used
> +to make it search alternate locations for the service config.
> +
> +@node tls_generate_server
> +@subsection Issuing server certificates
>
> Each server (or host) needs to be issued with a key and certificate. When
> connecting
> the certificate is sent to the client which validates it against the CA
> certificate.
> -The core piece of information for a server certificate is the hostname. This
> should
> -be the fully qualified hostname that the client will connect with, since the
> client
> -will typically also verify the hostname in the certificate. On the host
> holding the
> -secure CA private key:
> +The core pieces of information for a server certificate are the hostnames
> and/or IP
> +addresses that will be used by clients when connecting. The hostname / IP
> address
> +that the client specifies when connecting will be validated aganist the
> hostname(s)
s/aganist/against/
> +and IP address(es) recorded in the server certificate, and if no match is
> found
> +the client will close the connection.
> +
> +Thus it is recommended that the server certificate include both the fully
> qualfied
s/qualfied/qualified/
> +
> +If a single host is going to be using TLS in both a client and server
> +role, it is possible to create a single certificate to cover both roles.
> +This would be quite common for the migration and NBD services, where a
> +QEMU be start by accepting a TLS protected incoming migration, and later
s/QEMU be start/QEMU process will be started/
> +itself be migrated out to another host. To generate a single certificate,
> +simply include the template data from both the client and server
> +instructions in one.
>
>
> -When not using TLS the recommended configuration is
> +When copying the PEM files to the target host, save them twice
> +once as @code{server-cert.pem} and @code{server-key.pem}, and
s/twice/twice,/
> +against as @code{client-cert.pem} and @code{client-key.pem}.
s/against/again/
> +
> +@node tls_creds_setup
> +@subsection TLS x509 credential configuration
> +
> +QEMU has a standard mechanism for loading x509 credentials that will be
> +used for network services and clients. It requires specifying the
> +@code{tls-creds-x509} class name to the @code{-object} command line
> +argument for the system emulators. This also works for the helper tools
> +like @code{qemu-nbd} and @code{qemu-img}, but is named @code{--object}.
You can use '--object' with qemu as well (getopt_long_only() accepts
double-dash form in addition to single dash). If it makes it any easier
to only document the double-dash form, then go for it.
> +Each set of credentials loaded should be given a unique string identifier
> +via the @code{id} parameter. A single set of TLS credentials can be used
> +for multiple network backends, so VNC, migration, NBD, character devices
> +can all share the same credentials. Note, however, that credentials for
> +use in a client endpoint must be loaded separately from those used in
> +a server endpoint.
> +
> +When specifying the object, the @code{dir} parameters specifies which
> +directory contains the credential files. This directory is expected to
> +contain files with the names mentioned previously, @code{ca-cert.pem},
> +@code{server-key.pem}, @code{server-cert.pem}, @code{client-key.pem}
> +and @code{client-cert.pem} as appropriate. It is also possible to
> +include a set of pre-generated diffie-hellman parameters in a file
> +@code{dh-params.pem}, which can be created using the
> +@code{certtool --generate-dh-params} command. If omitted, QEMU will
> +dynamically generated DH parameters when loading the credentials.
s/generated/generate/
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3266
Virtualization: qemu.org | libvirt.org
signature.asc
Description: OpenPGP digital signature
Re: [Qemu-devel] in a device or CPU instance init/realize, can I rely on something having the BQL or equivalent?
On Fri, Dec 08, 2017 at 03:50:50PM +0100, Igor Mammedov wrote: > On Fri, 8 Dec 2017 13:19:27 + > Peter Maydell wrote: > > > On 8 December 2017 at 13:16, Igor Mammedov wrote: > > > TBH: > > > I do not recall why we have x86 max/host cpu types do feature > > > loading at realize time instead of at class init like the rest > > > of static cpu types. > > > > class init is too early, IIRC -- it's before KVM has been set up at all. > > that shouldn't be an issue as kvm_ppc_register_host_cpu_type() demonstrates > (i.e. an additional class init at kvm/tcg init time), It is possible, but IMO it's not a good idea. We should be able to enumerate all CPU types before the accelerator has been initialized, so query-cpu-definitions and "-cpu help" will always work. > > so it might be some compat issue or just legacy approach why it > havn't been rewritten to class_init for x86 the way PPC does. > But Eduardo probably knows better if there is anything left that > prevents using class init there. It's the opposite: x86 "host" CPU model used to work the same way as PPC, but we changed it so all classes are registered at type_init()-time. -- Eduardo
Re: [Qemu-devel] [PATCH 2/2] block/iscsi: only report an iSCSI Failure if we don't handle it gracefully
Am 08.12.2017 um 16:11 schrieb Eric Blake: > On 12/08/2017 05:51 AM, Peter Lieven wrote: >> we currently report an "iSCSI Failure" in iscsi_co_generic_cb if the task >> hasn't completed with SCSI_STATUS_GOOD. However, we expect a failure in >> some cases and handle it gracefully. This is the case for misaligned UNMAPs > Is the block layer still capable of producing a misaligned UNMAP? If > so, that's probably a bug in the block layer for not honoring the block > limit geometries. In theory there should be none. I think we can drop this code. Peter
Re: [Qemu-devel] [PATCH v7 for-2.12 24/25] block/curl: Implement bdrv_refresh_filename()
On Fri 08 Dec 2017 02:47:52 PM CET, Max Reitz wrote:
> +static void curl_refresh_filename(BlockDriverState *bs)
> +{
> +BDRVCURLState *s = bs->opaque;
> +
> +if (!s->sslverify || s->cookie ||
> +s->username || s->password || s->proxyusername ||
> s->proxypassword)
> +{
Is !s->sslverify negative because that setting is true by default?
>>>
>>> Yes, exactly. If it's false, you'd need to override it (and you can't
>>> do that through a plain filename).
>>
>> I think this is not the only case in this series, but I'm not very
>> comfortable with the idea that this condition and the default value
>> of the setting are implicity dependent on each other. If you change
>> one and forget to change the other things will break.
>
> Well, yes, but...
>
>> I understand that the default value is never supposed to change so in
>> practice I don't see this breaking,
>
> Yes.
>
>> but is it perhaps worth adding tests for all these cases?
>
> In theory, sure. In practice, adding a curl test case seems hard.
Indeed, I though it would perhaps be possible to create a curl BDS to
this without having to perform an actual connection, but never mind if
it's not possible / too complicated.
> Also, adding macros for the default values could help, I think.
Yep.
Berto
[Qemu-devel] [PATCH v3 for-2.12 14/14] s390x: change the QEMU cpu model to a stripped down z12
We are good enough to boot upstream Linux kernels / Fedora 26/27. That
should be sufficient for now.
As the QEMU CPU model is migration safe, let's add compatibility code.
Generate the feature list to reduce the chance of messing things up in the
future.
Signed-off-by: David Hildenbrand
---
hw/s390x/s390-virtio-ccw.c | 8
target/s390x/cpu.h | 3 ++
target/s390x/cpu_models.c | 100 ++--
target/s390x/cpu_models.h | 1 +
target/s390x/gen-features.c | 87 ++
5 files changed, 140 insertions(+), 59 deletions(-)
diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
index a23b8aec9f..9666fca04f 100644
--- a/hw/s390x/s390-virtio-ccw.c
+++ b/hw/s390x/s390-virtio-ccw.c
@@ -721,6 +721,10 @@ bool css_migration_enabled(void)
static void ccw_machine_2_12_instance_options(MachineState *machine)
{
+static const S390FeatBitmap qemu_cpu_feat = { S390_FEAT_LIST_QEMU_V2_12 };
+
+/* with 2.12 we emulated a stripped down zEC12 (GA 2) */
+s390_set_qemu_cpu_model(0x2827, 12, 2, qemu_cpu_feat);
}
static void ccw_machine_2_12_class_options(MachineClass *mc)
@@ -730,7 +734,11 @@ DEFINE_CCW_MACHINE(2_12, "2.12", true);
static void ccw_machine_2_11_instance_options(MachineState *machine)
{
+static const S390FeatBitmap qemu_cpu_feat = { S390_FEAT_LIST_QEMU_V2_11 };
ccw_machine_2_12_instance_options(machine);
+
+/* before 2.12 we emulated the very first z900 */
+s390_set_qemu_cpu_model(0x2064, 7, 1, qemu_cpu_feat);
}
static void ccw_machine_2_11_class_options(MachineClass *mc)
diff --git a/target/s390x/cpu.h b/target/s390x/cpu.h
index f9d4d62c48..6a91739ece 100644
--- a/target/s390x/cpu.h
+++ b/target/s390x/cpu.h
@@ -722,6 +722,9 @@ static inline unsigned int s390_cpu_set_state(uint8_t
cpu_state, S390CPU *cpu)
/* cpu_models.c */
void s390_cpu_list(FILE *f, fprintf_function cpu_fprintf);
#define cpu_list s390_cpu_list
+void s390_set_qemu_cpu_model(uint16_t type, uint8_t gen, uint8_t ec_ga,
+ const S390FeatBitmap features);
+
/* helper.c */
#define cpu_init(cpu_model) cpu_generic_init(TYPE_S390_CPU, cpu_model)
diff --git a/target/s390x/cpu_models.c b/target/s390x/cpu_models.c
index edac7fdecf..f0577f8155 100644
--- a/target/s390x/cpu_models.c
+++ b/target/s390x/cpu_models.c
@@ -15,7 +15,6 @@
#include "internal.h"
#include "kvm_s390x.h"
#include "sysemu/kvm.h"
-#include "gen-features.h"
#include "qapi/error.h"
#include "qapi/visitor.h"
#include "qemu/error-report.h"
@@ -81,6 +80,11 @@ static S390CPUDef s390_cpu_defs[] = {
CPUDEF_INIT(0x3906, 14, 1, 47, 0x0800U, "z14", "IBM z14 GA1"),
};
+#define QEMU_MAX_CPU_TYPE 0x2827
+#define QEMU_MAX_CPU_GEN 12
+#define QEMU_MAX_CPU_EC_GA 2
+static const S390FeatBitmap qemu_max_cpu_feat = { S390_FEAT_LIST_QEMU_MAX };
+
/* features part of a base model but not relevant for finding a base model */
S390FeatBitmap ignored_base_feat;
@@ -812,51 +816,6 @@ static void check_compatibility(const S390CPUModel
*max_model,
"available in the configuration: ");
}
-/**
- * The base TCG CPU model "qemu" is based on the z900. However, we already
- * can also emulate some additional features of later CPU generations, so
- * we add these additional feature bits here.
- */
-static void add_qemu_cpu_model_features(S390FeatBitmap fbm)
-{
-static const int feats[] = {
-S390_FEAT_DAT_ENH,
-S390_FEAT_IDTE_SEGMENT,
-S390_FEAT_STFLE,
-S390_FEAT_SENSE_RUNNING_STATUS,
-S390_FEAT_EXTENDED_TRANSLATION_2,
-S390_FEAT_MSA,
-S390_FEAT_LONG_DISPLACEMENT,
-S390_FEAT_LONG_DISPLACEMENT_FAST,
-S390_FEAT_EXTENDED_IMMEDIATE,
-S390_FEAT_EXTENDED_TRANSLATION_3,
-S390_FEAT_ETF2_ENH,
-S390_FEAT_STORE_CLOCK_FAST,
-S390_FEAT_MOVE_WITH_OPTIONAL_SPEC,
-S390_FEAT_ETF3_ENH,
-S390_FEAT_EXTRACT_CPU_TIME,
-S390_FEAT_COMPARE_AND_SWAP_AND_STORE,
-S390_FEAT_COMPARE_AND_SWAP_AND_STORE_2,
-S390_FEAT_GENERAL_INSTRUCTIONS_EXT,
-S390_FEAT_EXECUTE_EXT,
-S390_FEAT_SET_PROGRAM_PARAMETERS,
-S390_FEAT_FLOATING_POINT_SUPPPORT_ENH,
-S390_FEAT_STFLE_45,
-S390_FEAT_STFLE_49,
-S390_FEAT_LOCAL_TLB_CLEARING,
-S390_FEAT_INTERLOCKED_ACCESS_2,
-S390_FEAT_STFLE_53,
-S390_FEAT_MSA_EXT_5,
-S390_FEAT_MSA_EXT_3,
-S390_FEAT_MSA_EXT_4,
-};
-int i;
-
-for (i = 0; i < ARRAY_SIZE(feats); i++) {
-set_bit(feats[i], fbm);
-}
-}
-
static S390CPUModel *get_max_cpu_model(Error **errp)
{
static S390CPUModel max_model;
@@ -869,12 +828,10 @@ static S390CPUModel *get_max_cpu_model(Error **errp)
if (kvm_enabled()) {
kvm_s390_get_host_cpu_model(&max_model, errp);
} else {
-/* TCG emulates a z900 (with some optional additional features) */
-max_model.def = &s390_
[Qemu-devel] [PATCH v3 for-2.12 12/14] s390x/tcg: implement extract-CPU-time facility
It only provides the EXTRACT CPU TIME instruction. We can reuse the stpt
helper, which calculates the CPU timer value.
As the instruction is not privileged, but we don't have a CPU timer
value in case of linux user, we simply reuse cpu_get_host_ticks() to
produce some descending value.
Signed-off-by: David Hildenbrand
---
target/s390x/cpu_models.c | 1 +
target/s390x/helper.h | 2 +-
target/s390x/insn-data.def | 2 ++
target/s390x/misc_helper.c | 21 +++--
target/s390x/translate.c | 31 +++
5 files changed, 50 insertions(+), 7 deletions(-)
diff --git a/target/s390x/cpu_models.c b/target/s390x/cpu_models.c
index 94d24e423d..0be037eac1 100644
--- a/target/s390x/cpu_models.c
+++ b/target/s390x/cpu_models.c
@@ -834,6 +834,7 @@ static void add_qemu_cpu_model_features(S390FeatBitmap fbm)
S390_FEAT_STORE_CLOCK_FAST,
S390_FEAT_MOVE_WITH_OPTIONAL_SPEC,
S390_FEAT_ETF3_ENH,
+S390_FEAT_EXTRACT_CPU_TIME,
S390_FEAT_COMPARE_AND_SWAP_AND_STORE,
S390_FEAT_COMPARE_AND_SWAP_AND_STORE_2,
S390_FEAT_GENERAL_INSTRUCTIONS_EXT,
diff --git a/target/s390x/helper.h b/target/s390x/helper.h
index 102fbdd7b9..2f17b62d3d 100644
--- a/target/s390x/helper.h
+++ b/target/s390x/helper.h
@@ -119,6 +119,7 @@ DEF_HELPER_4(cu24, i32, env, i32, i32, i32)
DEF_HELPER_4(cu41, i32, env, i32, i32, i32)
DEF_HELPER_4(cu42, i32, env, i32, i32, i32)
DEF_HELPER_5(msa, i32, env, i32, i32, i32, i32)
+DEF_HELPER_FLAGS_1(stpt, TCG_CALL_NO_RWG, i64, env)
#ifndef CONFIG_USER_ONLY
DEF_HELPER_3(servc, i32, env, i64, i64)
@@ -130,7 +131,6 @@ DEF_HELPER_FLAGS_2(sckc, TCG_CALL_NO_RWG, void, env, i64)
DEF_HELPER_FLAGS_2(sckpf, TCG_CALL_NO_RWG, void, env, i64)
DEF_HELPER_FLAGS_1(stckc, TCG_CALL_NO_RWG, i64, env)
DEF_HELPER_FLAGS_2(spt, TCG_CALL_NO_RWG, void, env, i64)
-DEF_HELPER_FLAGS_1(stpt, TCG_CALL_NO_RWG, i64, env)
DEF_HELPER_4(stsi, i32, env, i64, i64, i64)
DEF_HELPER_FLAGS_4(lctl, TCG_CALL_NO_WG, void, env, i32, i64, i32)
DEF_HELPER_FLAGS_4(lctlg, TCG_CALL_NO_WG, void, env, i32, i64, i32)
diff --git a/target/s390x/insn-data.def b/target/s390x/insn-data.def
index 250741330d..11ee43dcbc 100644
--- a/target/s390x/insn-data.def
+++ b/target/s390x/insn-data.def
@@ -369,6 +369,8 @@
C(0xb24f, EAR, RRE, Z, 0, 0, new, r1_32, ear, 0)
/* EXTRACT CPU ATTRIBUTE */
C(0xeb4c, ECAG,RSY_a, GIE, 0, a2, r1, 0, ecag, 0)
+/* EXTRACT CPU TIME */
+C(0xc801, ECTG,SSF, ECT, 0, 0, 0, 0, ectg, 0)
/* EXTRACT FPC */
C(0xb38c, EFPC,RRE, Z, 0, 0, new, r1_32, efpc, 0)
/* EXTRACT PSW */
diff --git a/target/s390x/misc_helper.c b/target/s390x/misc_helper.c
index 7ddade2f0e..86da6aab7e 100644
--- a/target/s390x/misc_helper.c
+++ b/target/s390x/misc_helper.c
@@ -55,6 +55,21 @@ void HELPER(exception)(CPUS390XState *env, uint32_t excp)
cpu_loop_exit(cs);
}
+/* Store CPU Timer (also used for EXTRACT CPU TIME) */
+uint64_t HELPER(stpt)(CPUS390XState *env)
+{
+#if defined(CONFIG_USER_ONLY)
+/*
+ * Fake a descending CPU timer. We could get negative values here,
+ * but we don't care as it is up to the OS when to process that
+ * interrupt and reset to > 0.
+ */
+return UINT64_MAX - (uint64_t)cpu_get_host_ticks();
+#else
+return time2tod(env->cputm - qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL));
+#endif
+}
+
#ifndef CONFIG_USER_ONLY
/* SCLP service call */
@@ -178,12 +193,6 @@ void HELPER(spt)(CPUS390XState *env, uint64_t time)
timer_mod(env->cpu_timer, env->cputm);
}
-/* Store CPU Timer */
-uint64_t HELPER(stpt)(CPUS390XState *env)
-{
-return time2tod(env->cputm - qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL));
-}
-
/* Store System Information */
uint32_t HELPER(stsi)(CPUS390XState *env, uint64_t a0,
uint64_t r0, uint64_t r1)
diff --git a/target/s390x/translate.c b/target/s390x/translate.c
index 83e1df0f48..eede2ed157 100644
--- a/target/s390x/translate.c
+++ b/target/s390x/translate.c
@@ -3917,6 +3917,36 @@ static ExitStatus op_spm(DisasContext *s, DisasOps *o)
return NO_EXIT;
}
+static ExitStatus op_ectg(DisasContext *s, DisasOps *o)
+{
+int b1 = get_field(s->fields, b1);
+int d1 = get_field(s->fields, d1);
+int b2 = get_field(s->fields, b2);
+int d2 = get_field(s->fields, d2);
+int r3 = get_field(s->fields, r3);
+TCGv_i64 tmp = tcg_temp_new_i64();
+
+/* fetch all operands first */
+o->in1 = tcg_temp_new_i64();
+tcg_gen_addi_i64(o->in1, regs[b1], d1);
+o->in2 = tcg_temp_new_i64();
+tcg_gen_addi_i64(o->in2, regs[b2], d2);
+o->addr1 = get_address(s, 0, r3, 0);
+
+/* load the third operand into r3 before modifying anything */
+tcg_gen_qemu_ld64(regs[r3], o->addr1, get_mem_index(s));
+
+/* subtract CPU timer from first operand and store in GR0 */
+gen_helper_stpt(tmp, cpu_env);
+tcg_gen_sub_i64(regs[0], o->in1, tmp);
+
+/* store second operand in GR1 */
+tcg_gen_mov_i64(regs[1], o->
[Qemu-devel] [PATCH v3 for-2.12 10/14] s390x/tcg: Implement STORE CHANNEL PATH STATUS
Just like KVM does, we should suppress this instruction:
When this instruction is not provided, it is
checked for privileged operation exception and the
instruction is suppressed by the machine
Reviewed-by: Richard Henderson
Signed-off-by: David Hildenbrand
---
target/s390x/insn-data.def | 1 +
target/s390x/translate.c | 7 +++
2 files changed, 8 insertions(+)
diff --git a/target/s390x/insn-data.def b/target/s390x/insn-data.def
index 0c225d5e78..2e47a6b5bc 100644
--- a/target/s390x/insn-data.def
+++ b/target/s390x/insn-data.def
@@ -1056,6 +1056,7 @@
C(0xb238, RSCH,S, Z, 0, 0, 0, 0, rsch, 0)
C(0xb237, SAL, S, Z, 0, 0, 0, 0, sal, 0)
C(0xb23c, SCHM,S, Z, 0, insn, 0, 0, schm, 0)
+C(0xb23a, STCPS, S, Z, 0, 0, 0, 0, stcps, 0)
C(0xb233, SSCH,S, Z, 0, insn, 0, 0, ssch, 0)
C(0xb239, STCRW, S, Z, 0, insn, 0, 0, stcrw, 0)
C(0xb234, STSCH, S, Z, 0, insn, 0, 0, stsch, 0)
diff --git a/target/s390x/translate.c b/target/s390x/translate.c
index 8cf35a7b49..16febf4274 100644
--- a/target/s390x/translate.c
+++ b/target/s390x/translate.c
@@ -4132,6 +4132,13 @@ static ExitStatus op_schm(DisasContext *s, DisasOps *o)
return NO_EXIT;
}
+static ExitStatus op_stcps(DisasContext *s, DisasOps *o)
+{
+check_privileged(s);
+/* The instruction is suppressed if not provided. */
+return NO_EXIT;
+}
+
static ExitStatus op_ssch(DisasContext *s, DisasOps *o)
{
check_privileged(s);
--
2.14.3
[Qemu-devel] [PATCH v3 for-2.12 09/14] s390x/tcg: wire up SET CHANNEL MONITOR
Let's just wire it up like KVM.
Reviewed-by: Richard Henderson
Signed-off-by: David Hildenbrand
---
target/s390x/helper.h | 1 +
target/s390x/insn-data.def | 1 +
target/s390x/misc_helper.c | 9 +
target/s390x/translate.c | 7 +++
4 files changed, 18 insertions(+)
diff --git a/target/s390x/helper.h b/target/s390x/helper.h
index f1acc34f36..102fbdd7b9 100644
--- a/target/s390x/helper.h
+++ b/target/s390x/helper.h
@@ -166,6 +166,7 @@ DEF_HELPER_3(msch, void, env, i64, i64)
DEF_HELPER_2(rchp, void, env, i64)
DEF_HELPER_2(rsch, void, env, i64)
DEF_HELPER_2(sal, void, env, i64)
+DEF_HELPER_4(schm, void, env, i64, i64, i64)
DEF_HELPER_3(ssch, void, env, i64, i64)
DEF_HELPER_2(stcrw, void, env, i64)
DEF_HELPER_3(stsch, void, env, i64, i64)
diff --git a/target/s390x/insn-data.def b/target/s390x/insn-data.def
index 11746f5298..0c225d5e78 100644
--- a/target/s390x/insn-data.def
+++ b/target/s390x/insn-data.def
@@ -1055,6 +1055,7 @@
C(0xb23b, RCHP,S, Z, 0, 0, 0, 0, rchp, 0)
C(0xb238, RSCH,S, Z, 0, 0, 0, 0, rsch, 0)
C(0xb237, SAL, S, Z, 0, 0, 0, 0, sal, 0)
+C(0xb23c, SCHM,S, Z, 0, insn, 0, 0, schm, 0)
C(0xb233, SSCH,S, Z, 0, insn, 0, 0, ssch, 0)
C(0xb239, STCRW, S, Z, 0, insn, 0, 0, stcrw, 0)
C(0xb234, STSCH, S, Z, 0, insn, 0, 0, stsch, 0)
diff --git a/target/s390x/misc_helper.c b/target/s390x/misc_helper.c
index 1941c9c3de..7ddade2f0e 100644
--- a/target/s390x/misc_helper.c
+++ b/target/s390x/misc_helper.c
@@ -386,6 +386,15 @@ void HELPER(sal)(CPUS390XState *env, uint64_t r1)
qemu_mutex_unlock_iothread();
}
+void HELPER(schm)(CPUS390XState *env, uint64_t r1, uint64_t r2, uint64_t inst)
+{
+S390CPU *cpu = s390_env_get_cpu(env);
+
+qemu_mutex_lock_iothread();
+ioinst_handle_schm(cpu, r1, r2, inst >> 16, GETPC());
+qemu_mutex_unlock_iothread();
+}
+
void HELPER(ssch)(CPUS390XState *env, uint64_t r1, uint64_t inst)
{
S390CPU *cpu = s390_env_get_cpu(env);
diff --git a/target/s390x/translate.c b/target/s390x/translate.c
index 30d3d767ea..8cf35a7b49 100644
--- a/target/s390x/translate.c
+++ b/target/s390x/translate.c
@@ -4125,6 +4125,13 @@ static ExitStatus op_sal(DisasContext *s, DisasOps *o)
return NO_EXIT;
}
+static ExitStatus op_schm(DisasContext *s, DisasOps *o)
+{
+check_privileged(s);
+gen_helper_schm(cpu_env, regs[1], regs[2], o->in2);
+return NO_EXIT;
+}
+
static ExitStatus op_ssch(DisasContext *s, DisasOps *o)
{
check_privileged(s);
--
2.14.3
[Qemu-devel] [PATCH v3 for-2.12 11/14] s390x/tcg: Implement SIGNAL ADAPTER instruction
KVM suppresses SIGA, setting cc=3. Let's do the same for TCG, so we're at
least equal.
Reviewed-by: Richard Henderson
Signed-off-by: David Hildenbrand
---
target/s390x/insn-data.def | 1 +
target/s390x/translate.c | 8
2 files changed, 9 insertions(+)
diff --git a/target/s390x/insn-data.def b/target/s390x/insn-data.def
index 2e47a6b5bc..250741330d 100644
--- a/target/s390x/insn-data.def
+++ b/target/s390x/insn-data.def
@@ -1056,6 +1056,7 @@
C(0xb238, RSCH,S, Z, 0, 0, 0, 0, rsch, 0)
C(0xb237, SAL, S, Z, 0, 0, 0, 0, sal, 0)
C(0xb23c, SCHM,S, Z, 0, insn, 0, 0, schm, 0)
+C(0xb274, SIGA,S, Z, 0, 0, 0, 0, siga, 0)
C(0xb23a, STCPS, S, Z, 0, 0, 0, 0, stcps, 0)
C(0xb233, SSCH,S, Z, 0, insn, 0, 0, ssch, 0)
C(0xb239, STCRW, S, Z, 0, insn, 0, 0, stcrw, 0)
diff --git a/target/s390x/translate.c b/target/s390x/translate.c
index 16febf4274..83e1df0f48 100644
--- a/target/s390x/translate.c
+++ b/target/s390x/translate.c
@@ -4132,6 +4132,14 @@ static ExitStatus op_schm(DisasContext *s, DisasOps *o)
return NO_EXIT;
}
+static ExitStatus op_siga(DisasContext *s, DisasOps *o)
+{
+check_privileged(s);
+/* From KVM code: Not provided, set CC = 3 for subchannel not operational
*/
+gen_op_movi_cc(s, 3);
+return NO_EXIT;
+}
+
static ExitStatus op_stcps(DisasContext *s, DisasOps *o)
{
check_privileged(s);
--
2.14.3
[Qemu-devel] [PATCH v3 for-2.12 13/14] s390x/tcg: we already implement the Set-Program-Parameter facility
The Set-Program-Parameter facility (also known as Load-Program-Parameter facility) provides the LPP instruction used to load the program parameter. We already implement that instruction in TCG, so add it to our list. Note: Not documented in the PoP but in "The Load-Program-Parameter and CPU-Measurement Facilities) - SA23-2260-05 document. While at it, make the whole list ordered (according to cpu_features_def.h). Reviewed-by: Richard Henderson Signed-off-by: David Hildenbrand --- target/s390x/cpu_models.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/target/s390x/cpu_models.c b/target/s390x/cpu_models.c index 0be037eac1..edac7fdecf 100644 --- a/target/s390x/cpu_models.c +++ b/target/s390x/cpu_models.c @@ -824,12 +824,12 @@ static void add_qemu_cpu_model_features(S390FeatBitmap fbm) S390_FEAT_IDTE_SEGMENT, S390_FEAT_STFLE, S390_FEAT_SENSE_RUNNING_STATUS, -S390_FEAT_EXTENDED_IMMEDIATE, S390_FEAT_EXTENDED_TRANSLATION_2, S390_FEAT_MSA, -S390_FEAT_EXTENDED_TRANSLATION_3, S390_FEAT_LONG_DISPLACEMENT, S390_FEAT_LONG_DISPLACEMENT_FAST, +S390_FEAT_EXTENDED_IMMEDIATE, +S390_FEAT_EXTENDED_TRANSLATION_3, S390_FEAT_ETF2_ENH, S390_FEAT_STORE_CLOCK_FAST, S390_FEAT_MOVE_WITH_OPTIONAL_SPEC, @@ -839,6 +839,7 @@ static void add_qemu_cpu_model_features(S390FeatBitmap fbm) S390_FEAT_COMPARE_AND_SWAP_AND_STORE_2, S390_FEAT_GENERAL_INSTRUCTIONS_EXT, S390_FEAT_EXECUTE_EXT, +S390_FEAT_SET_PROGRAM_PARAMETERS, S390_FEAT_FLOATING_POINT_SUPPPORT_ENH, S390_FEAT_STFLE_45, S390_FEAT_STFLE_49, -- 2.14.3
[Qemu-devel] [PATCH v3 for-2.12 08/14] s390x/tcg: wire up SET ADDRESS LIMIT
Let's handle it just like KVM:
Depending on the model, this instruction may not be
provided. When this instruction is not provided, it is
checked for operand exception and privileged-opera-
tion exception, and then is suppressed.
Reviewed-by: Richard Henderson
Signed-off-by: David Hildenbrand
---
target/s390x/helper.h | 1 +
target/s390x/insn-data.def | 1 +
target/s390x/misc_helper.c | 9 +
target/s390x/translate.c | 7 +++
4 files changed, 18 insertions(+)
diff --git a/target/s390x/helper.h b/target/s390x/helper.h
index ba11cfdc30..f1acc34f36 100644
--- a/target/s390x/helper.h
+++ b/target/s390x/helper.h
@@ -165,6 +165,7 @@ DEF_HELPER_2(hsch, void, env, i64)
DEF_HELPER_3(msch, void, env, i64, i64)
DEF_HELPER_2(rchp, void, env, i64)
DEF_HELPER_2(rsch, void, env, i64)
+DEF_HELPER_2(sal, void, env, i64)
DEF_HELPER_3(ssch, void, env, i64, i64)
DEF_HELPER_2(stcrw, void, env, i64)
DEF_HELPER_3(stsch, void, env, i64, i64)
diff --git a/target/s390x/insn-data.def b/target/s390x/insn-data.def
index 6cbd604814..11746f5298 100644
--- a/target/s390x/insn-data.def
+++ b/target/s390x/insn-data.def
@@ -1054,6 +1054,7 @@
C(0xb232, MSCH,S, Z, 0, insn, 0, 0, msch, 0)
C(0xb23b, RCHP,S, Z, 0, 0, 0, 0, rchp, 0)
C(0xb238, RSCH,S, Z, 0, 0, 0, 0, rsch, 0)
+C(0xb237, SAL, S, Z, 0, 0, 0, 0, sal, 0)
C(0xb233, SSCH,S, Z, 0, insn, 0, 0, ssch, 0)
C(0xb239, STCRW, S, Z, 0, insn, 0, 0, stcrw, 0)
C(0xb234, STSCH, S, Z, 0, insn, 0, 0, stsch, 0)
diff --git a/target/s390x/misc_helper.c b/target/s390x/misc_helper.c
index 04fb53d8a3..1941c9c3de 100644
--- a/target/s390x/misc_helper.c
+++ b/target/s390x/misc_helper.c
@@ -377,6 +377,15 @@ void HELPER(rsch)(CPUS390XState *env, uint64_t r1)
qemu_mutex_unlock_iothread();
}
+void HELPER(sal)(CPUS390XState *env, uint64_t r1)
+{
+S390CPU *cpu = s390_env_get_cpu(env);
+
+qemu_mutex_lock_iothread();
+ioinst_handle_sal(cpu, r1, GETPC());
+qemu_mutex_unlock_iothread();
+}
+
void HELPER(ssch)(CPUS390XState *env, uint64_t r1, uint64_t inst)
{
S390CPU *cpu = s390_env_get_cpu(env);
diff --git a/target/s390x/translate.c b/target/s390x/translate.c
index de00b9471a..30d3d767ea 100644
--- a/target/s390x/translate.c
+++ b/target/s390x/translate.c
@@ -4118,6 +4118,13 @@ static ExitStatus op_rsch(DisasContext *s, DisasOps *o)
return NO_EXIT;
}
+static ExitStatus op_sal(DisasContext *s, DisasOps *o)
+{
+check_privileged(s);
+gen_helper_sal(cpu_env, regs[1]);
+return NO_EXIT;
+}
+
static ExitStatus op_ssch(DisasContext *s, DisasOps *o)
{
check_privileged(s);
--
2.14.3
[Qemu-devel] [PATCH v3 for-2.12 07/14] s390x/tcg: implement Interlocked-Access Facility 2
With this facility, OI/OIY, NI/NIY and XI/XIY are atomic. All operate on
one byte (MO_UB). Emulate old behavior.
Signed-off-by: David Hildenbrand
---
target/s390x/cpu_models.c | 1 +
target/s390x/insn-data.def | 12 -
target/s390x/translate.c | 63 ++
3 files changed, 70 insertions(+), 6 deletions(-)
diff --git a/target/s390x/cpu_models.c b/target/s390x/cpu_models.c
index c4c37b3b15..94d24e423d 100644
--- a/target/s390x/cpu_models.c
+++ b/target/s390x/cpu_models.c
@@ -842,6 +842,7 @@ static void add_qemu_cpu_model_features(S390FeatBitmap fbm)
S390_FEAT_STFLE_45,
S390_FEAT_STFLE_49,
S390_FEAT_LOCAL_TLB_CLEARING,
+S390_FEAT_INTERLOCKED_ACCESS_2,
S390_FEAT_STFLE_53,
S390_FEAT_MSA_EXT_5,
S390_FEAT_MSA_EXT_3,
diff --git a/target/s390x/insn-data.def b/target/s390x/insn-data.def
index 166ee7c80b..6cbd604814 100644
--- a/target/s390x/insn-data.def
+++ b/target/s390x/insn-data.def
@@ -99,8 +99,8 @@
D(0xa505, NIHL,RI_a, Z, r1_o, i2_16u, r1, 0, andi, 0, 0x1020)
D(0xa506, NILH,RI_a, Z, r1_o, i2_16u, r1, 0, andi, 0, 0x1010)
D(0xa507, NILL,RI_a, Z, r1_o, i2_16u, r1, 0, andi, 0, 0x1000)
-C(0x9400, NI, SI,Z, m1_8u, i2_8u, new, m1_8, and, nz64)
-C(0xeb54, NIY, SIY, LD, m1_8u, i2_8u, new, m1_8, and, nz64)
+D(0x9400, NI, SI,Z, la1, i2_8u, new, 0, ni, nz64, MO_UB)
+D(0xeb54, NIY, SIY, LD, la1, i2_8u, new, 0, ni, nz64, MO_UB)
/* BRANCH AND SAVE */
C(0x0d00, BASR,RR_a, Z, 0, r2_nz, r1, 0, bas, 0)
@@ -357,8 +357,8 @@
/* EXCLUSIVE OR IMMEDIATE */
D(0xc006, XIHF,RIL_a, EI, r1_o, i2_32u, r1, 0, xori, 0, 0x2020)
D(0xc007, XILF,RIL_a, EI, r1_o, i2_32u, r1, 0, xori, 0, 0x2000)
-C(0x9700, XI, SI,Z, m1_8u, i2_8u, new, m1_8, xor, nz64)
-C(0xeb57, XIY, SIY, LD, m1_8u, i2_8u, new, m1_8, xor, nz64)
+D(0x9700, XI, SI,Z, la1, i2_8u, new, 0, xi, nz64, MO_UB)
+D(0xeb57, XIY, SIY, LD, la1, i2_8u, new, 0, xi, nz64, MO_UB)
/* EXECUTE */
C(0x4400, EX, RX_a, Z, 0, a2, 0, 0, ex, 0)
@@ -698,8 +698,8 @@
D(0xa509, OIHL,RI_a, Z, r1_o, i2_16u, r1, 0, ori, 0, 0x1020)
D(0xa50a, OILH,RI_a, Z, r1_o, i2_16u, r1, 0, ori, 0, 0x1010)
D(0xa50b, OILL,RI_a, Z, r1_o, i2_16u, r1, 0, ori, 0, 0x1000)
-C(0x9600, OI, SI,Z, m1_8u, i2_8u, new, m1_8, or, nz64)
-C(0xeb56, OIY, SIY, LD, m1_8u, i2_8u, new, m1_8, or, nz64)
+D(0x9600, OI, SI,Z, la1, i2_8u, new, 0, oi, nz64, MO_UB)
+D(0xeb56, OIY, SIY, LD, la1, i2_8u, new, 0, oi, nz64, MO_UB)
/* PACK */
/* Really format SS_b, but we pack both lengths into one argument
diff --git a/target/s390x/translate.c b/target/s390x/translate.c
index 7ab8e853ab..de00b9471a 100644
--- a/target/s390x/translate.c
+++ b/target/s390x/translate.c
@@ -1427,6 +1427,27 @@ static ExitStatus op_andi(DisasContext *s, DisasOps *o)
return NO_EXIT;
}
+static ExitStatus op_ni(DisasContext *s, DisasOps *o)
+{
+o->in1 = tcg_temp_new_i64();
+
+if (!s390_has_feat(S390_FEAT_INTERLOCKED_ACCESS_2)) {
+tcg_gen_qemu_ld_tl(o->in1, o->addr1, get_mem_index(s), s->insn->data);
+} else {
+/* Perform the atomic operation in memory. */
+tcg_gen_atomic_fetch_and_i64(o->in1, o->addr1, o->in2,
get_mem_index(s),
+ s->insn->data);
+}
+
+/* Recompute also for atomic case: needed for setting CC. */
+tcg_gen_and_i64(o->out, o->in1, o->in2);
+
+if (!s390_has_feat(S390_FEAT_INTERLOCKED_ACCESS_2)) {
+tcg_gen_qemu_st_tl(o->out, o->addr1, get_mem_index(s), s->insn->data);
+}
+return NO_EXIT;
+}
+
static ExitStatus op_bas(DisasContext *s, DisasOps *o)
{
tcg_gen_movi_i64(o->out, pc_to_link_info(s, s->next_pc));
@@ -3378,6 +3399,27 @@ static ExitStatus op_ori(DisasContext *s, DisasOps *o)
return NO_EXIT;
}
+static ExitStatus op_oi(DisasContext *s, DisasOps *o)
+{
+o->in1 = tcg_temp_new_i64();
+
+if (!s390_has_feat(S390_FEAT_INTERLOCKED_ACCESS_2)) {
+tcg_gen_qemu_ld_tl(o->in1, o->addr1, get_mem_index(s), s->insn->data);
+} else {
+/* Perform the atomic operation in memory. */
+tcg_gen_atomic_fetch_or_i64(o->in1, o->addr1, o->in2, get_mem_index(s),
+s->insn->data);
+}
+
+/* Recompute also for atomic case: needed for setting CC. */
+tcg_gen_or_i64(o->out, o->in1, o->in2);
+
+if (!s390_has_feat(S390_FEAT_INTERLOCKED_ACCESS_2)) {
+tcg_gen_qemu_st_tl(o->out, o->addr1, get_mem_index(s), s->insn->data);
+}
+return NO_EXIT;
+}
+
static ExitStatus op_pack(DisasContext *s, DisasOps *o)
{
TCGv_i32 l = tcg_const_i32(get_field(s->fields, l1));
@@ -4643,6 +4685,27 @@ static ExitStatus op_xori(DisasContext *s, DisasOps *o)
return NO_EXIT;
}
+static ExitStatus op
[Qemu-devel] [PATCH v3 for-2.12 06/14] s390x/tcg: ASI/ASGI/ALSI/ALSGI are atomic with Interlocked-acccess facility 1
The semantics of ASI/ASGI/ALSI/ALSGI changed. Let's implement them just
like LOAD AND ADD, so they are atomic. Emulate old behavior.
This fixes random crashes when booting a Linux kernel compiled for
z196+ with SMP + MTTCG.
Signed-off-by: David Hildenbrand
---
target/s390x/insn-data.def | 8
target/s390x/translate.c | 21 +
2 files changed, 25 insertions(+), 4 deletions(-)
diff --git a/target/s390x/insn-data.def b/target/s390x/insn-data.def
index 43ab1963c8..166ee7c80b 100644
--- a/target/s390x/insn-data.def
+++ b/target/s390x/insn-data.def
@@ -39,10 +39,10 @@
C(0xb9d8, AHHLR, RRF_a, HW, r2_sr32, r3, new, r1_32h, add, adds32)
/* ADD IMMEDIATE */
C(0xc209, AFI, RIL_a, EI, r1, i2, new, r1_32, add, adds32)
-C(0xeb6a, ASI, SIY, GIE, m1_32s, i2, new, m1_32, add, adds32)
+D(0xeb6a, ASI, SIY, GIE, la1, i2, new, 0, asi, adds32, MO_TESL)
C(0xecd8, AHIK,RIE_d, DO, r3, i2, new, r1_32, add, adds32)
C(0xc208, AGFI,RIL_a, EI, r1, i2, r1, 0, add, adds64)
-C(0xeb7a, AGSI,SIY, GIE, m1_64, i2, new, m1_64, add, adds64)
+D(0xeb7a, AGSI,SIY, GIE, la1, i2, new, 0, asi, adds64, MO_TEQ)
C(0xecd9, AGHIK, RIE_d, DO, r3, i2, r1, 0, add, adds64)
/* ADD IMMEDIATE HIGH */
C(0xcc08, AIH, RIL_a, HW, r1_sr32, i2, new, r1_32h, add, adds32)
@@ -70,9 +70,9 @@
C(0xc20b, ALFI,RIL_a, EI, r1, i2_32u, new, r1_32, add, addu32)
C(0xc20a, ALGFI, RIL_a, EI, r1, i2_32u, r1, 0, add, addu64)
/* ADD LOGICAL WITH SIGNED IMMEDIATE */
-C(0xeb6e, ALSI,SIY, GIE, m1_32u, i2, new, m1_32, add, addu32)
+D(0xeb6e, ALSI,SIY, GIE, la1, i2, new, 0, asi, addu32, MO_TEUL)
C(0xecda, ALHSIK, RIE_d, DO, r3, i2, new, r1_32, add, addu32)
-C(0xeb7e, ALGSI, SIY, GIE, m1_64, i2, new, m1_64, add, addu64)
+D(0xeb7e, ALGSI, SIY, GIE, la1, i2, new, 0, asi, addu64, MO_TEQ)
C(0xecdb, ALGHSIK, RIE_d, DO, r3, i2, r1, 0, add, addu64)
/* ADD LOGICAL WITH SIGNED IMMEDIATE HIGH */
C(0xcc0a, ALSIH, RIL_a, HW, r1_sr32, i2, new, r1_32h, add, addu32)
diff --git a/target/s390x/translate.c b/target/s390x/translate.c
index 08c1ace0d8..7ab8e853ab 100644
--- a/target/s390x/translate.c
+++ b/target/s390x/translate.c
@@ -1364,6 +1364,27 @@ static ExitStatus op_addc(DisasContext *s, DisasOps *o)
return NO_EXIT;
}
+static ExitStatus op_asi(DisasContext *s, DisasOps *o)
+{
+o->in1 = tcg_temp_new_i64();
+
+if (!s390_has_feat(S390_FEAT_STFLE_45)) {
+tcg_gen_qemu_ld_tl(o->in1, o->addr1, get_mem_index(s), s->insn->data);
+} else {
+/* Perform the atomic addition in memory. */
+tcg_gen_atomic_fetch_add_i64(o->in1, o->addr1, o->in2,
get_mem_index(s),
+ s->insn->data);
+}
+
+/* Recompute also for atomic case: needed for setting CC. */
+tcg_gen_add_i64(o->out, o->in1, o->in2);
+
+if (!s390_has_feat(S390_FEAT_STFLE_45)) {
+tcg_gen_qemu_st_tl(o->out, o->addr1, get_mem_index(s), s->insn->data);
+}
+return NO_EXIT;
+}
+
static ExitStatus op_aeb(DisasContext *s, DisasOps *o)
{
gen_helper_aeb(o->out, cpu_env, o->in1, o->in2);
--
2.14.3
[Qemu-devel] [PATCH v3 for-2.12 05/14] s390x/tcg: wire up STORE CHANNEL REPORT WORD
CRW machine check handling requires STCRW. So let's wire it up.
Reviewed-by: Thomas Huth
Reviewed-by: Richard Henderson
Signed-off-by: David Hildenbrand
---
target/s390x/helper.h | 1 +
target/s390x/insn-data.def | 1 +
target/s390x/misc_helper.c | 9 +
target/s390x/translate.c | 8
4 files changed, 19 insertions(+)
diff --git a/target/s390x/helper.h b/target/s390x/helper.h
index 3eb7715e5b..ba11cfdc30 100644
--- a/target/s390x/helper.h
+++ b/target/s390x/helper.h
@@ -166,6 +166,7 @@ DEF_HELPER_3(msch, void, env, i64, i64)
DEF_HELPER_2(rchp, void, env, i64)
DEF_HELPER_2(rsch, void, env, i64)
DEF_HELPER_3(ssch, void, env, i64, i64)
+DEF_HELPER_2(stcrw, void, env, i64)
DEF_HELPER_3(stsch, void, env, i64, i64)
DEF_HELPER_3(tsch, void, env, i64, i64)
DEF_HELPER_2(chsc, void, env, i64)
diff --git a/target/s390x/insn-data.def b/target/s390x/insn-data.def
index 8c2541f545..43ab1963c8 100644
--- a/target/s390x/insn-data.def
+++ b/target/s390x/insn-data.def
@@ -1055,6 +1055,7 @@
C(0xb23b, RCHP,S, Z, 0, 0, 0, 0, rchp, 0)
C(0xb238, RSCH,S, Z, 0, 0, 0, 0, rsch, 0)
C(0xb233, SSCH,S, Z, 0, insn, 0, 0, ssch, 0)
+C(0xb239, STCRW, S, Z, 0, insn, 0, 0, stcrw, 0)
C(0xb234, STSCH, S, Z, 0, insn, 0, 0, stsch, 0)
C(0xb235, TSCH,S, Z, 0, insn, 0, 0, tsch, 0)
/* ??? Not listed in PoO ninth edition, but there's a linux driver that
diff --git a/target/s390x/misc_helper.c b/target/s390x/misc_helper.c
index 769ec52e1d..04fb53d8a3 100644
--- a/target/s390x/misc_helper.c
+++ b/target/s390x/misc_helper.c
@@ -385,6 +385,15 @@ void HELPER(ssch)(CPUS390XState *env, uint64_t r1,
uint64_t inst)
qemu_mutex_unlock_iothread();
}
+void HELPER(stcrw)(CPUS390XState *env, uint64_t inst)
+{
+S390CPU *cpu = s390_env_get_cpu(env);
+
+qemu_mutex_lock_iothread();
+ioinst_handle_stcrw(cpu, inst >> 16, GETPC());
+qemu_mutex_unlock_iothread();
+}
+
void HELPER(stsch)(CPUS390XState *env, uint64_t r1, uint64_t inst)
{
S390CPU *cpu = s390_env_get_cpu(env);
diff --git a/target/s390x/translate.c b/target/s390x/translate.c
index 38e1770e5e..08c1ace0d8 100644
--- a/target/s390x/translate.c
+++ b/target/s390x/translate.c
@@ -4071,6 +4071,14 @@ static ExitStatus op_stsch(DisasContext *s, DisasOps *o)
return NO_EXIT;
}
+static ExitStatus op_stcrw(DisasContext *s, DisasOps *o)
+{
+check_privileged(s);
+gen_helper_stcrw(cpu_env, o->in2);
+set_cc_static(s);
+return NO_EXIT;
+}
+
static ExitStatus op_tsch(DisasContext *s, DisasOps *o)
{
check_privileged(s);
--
2.14.3
[Qemu-devel] [PATCH v3 for-2.12 03/14] s390x/tcg: implement SET CLOCK PROGRAMMABLE FIELD
Needed for machine check handling inside Linux (when restoring registers).
Except for SIGP and machine checks, we don't make use of the register
yet. Sufficient for now.
Reviewed-by: Thomas Huth
Signed-off-by: David Hildenbrand
---
target/s390x/helper.h | 1 +
target/s390x/insn-data.def | 2 ++
target/s390x/misc_helper.c | 11 +++
target/s390x/translate.c | 7 +++
4 files changed, 21 insertions(+)
diff --git a/target/s390x/helper.h b/target/s390x/helper.h
index 9459b73c73..3eb7715e5b 100644
--- a/target/s390x/helper.h
+++ b/target/s390x/helper.h
@@ -127,6 +127,7 @@ DEF_HELPER_3(load_psw, noreturn, env, i64, i64)
DEF_HELPER_FLAGS_2(spx, TCG_CALL_NO_RWG, void, env, i64)
DEF_HELPER_FLAGS_1(stck, TCG_CALL_NO_RWG_SE, i64, env)
DEF_HELPER_FLAGS_2(sckc, TCG_CALL_NO_RWG, void, env, i64)
+DEF_HELPER_FLAGS_2(sckpf, TCG_CALL_NO_RWG, void, env, i64)
DEF_HELPER_FLAGS_1(stckc, TCG_CALL_NO_RWG, i64, env)
DEF_HELPER_FLAGS_2(spt, TCG_CALL_NO_RWG, void, env, i64)
DEF_HELPER_FLAGS_1(stpt, TCG_CALL_NO_RWG, i64, env)
diff --git a/target/s390x/insn-data.def b/target/s390x/insn-data.def
index 16e27c8a35..8c2541f545 100644
--- a/target/s390x/insn-data.def
+++ b/target/s390x/insn-data.def
@@ -999,6 +999,8 @@
C(0xb204, SCK, S, Z, 0, 0, 0, 0, 0, 0)
/* SET CLOCK COMPARATOR */
C(0xb206, SCKC,S, Z, 0, m2_64, 0, 0, sckc, 0)
+/* SET CLOCK PROGRAMMABLE FIELD */
+C(0x0107, SCKPF, E, Z, 0, 0, 0, 0, sckpf, 0)
/* SET CPU TIMER */
C(0xb208, SPT, S, Z, 0, m2_64, 0, 0, spt, 0)
/* SET PREFIX */
diff --git a/target/s390x/misc_helper.c b/target/s390x/misc_helper.c
index 6d766ce1e7..769ec52e1d 100644
--- a/target/s390x/misc_helper.c
+++ b/target/s390x/misc_helper.c
@@ -146,6 +146,17 @@ void HELPER(sckc)(CPUS390XState *env, uint64_t time)
timer_mod(env->tod_timer, env->tod_basetime + time);
}
+/* Set Tod Programmable Field */
+void HELPER(sckpf)(CPUS390XState *env, uint64_t r0)
+{
+uint32_t val = r0;
+
+if (val & 0x) {
+s390_program_interrupt(env, PGM_SPECIFICATION, 2, GETPC());
+}
+env->todpr = val;
+}
+
/* Store Clock Comparator */
uint64_t HELPER(stckc)(CPUS390XState *env)
{
diff --git a/target/s390x/translate.c b/target/s390x/translate.c
index 26cf993405..d13f531c5b 100644
--- a/target/s390x/translate.c
+++ b/target/s390x/translate.c
@@ -3922,6 +3922,13 @@ static ExitStatus op_sckc(DisasContext *s, DisasOps *o)
return NO_EXIT;
}
+static ExitStatus op_sckpf(DisasContext *s, DisasOps *o)
+{
+check_privileged(s);
+gen_helper_sckpf(cpu_env, regs[0]);
+return NO_EXIT;
+}
+
static ExitStatus op_stckc(DisasContext *s, DisasOps *o)
{
check_privileged(s);
--
2.14.3
[Qemu-devel] [PATCH v3 for-2.12 00/14] s390x/tcg: CCW hotplug, facilities, instructions
Both series in one piece as (most probably) most reviewing is done. Wire up some io instructions and implement new facilitites. Make sure to take care of MTTCG when it comes to atomic operations. Make CCW hotplug work. As we are now able to install/boot a Fedora 26/27 as well as an upstream kernel compiled for z12, let's bump up the QEMU cpu model to a very stripped down version of a z12 (with missing base features). Take care of backwards compatibility (as we defined the QEMU model as migration-safe). Avilable at: https://github.com/davidhildenbrand/qemu.git s390x-queue v2 -> v3: - squashed the ASI/ASGI and ALSI/ALSGI patches. - Both now emulate old behavior. - "s390x/tcg: implement Interlocked-Access Facility 2" - now also emulates old behavior. - "s390x/tcg: implement SET CLOCK PROGRAMMABLE FIELD" - now forwards r1 into the helper. - "s390x/tcg: implement extract-CPU-time facility" - takes care of user-only - "s390x: change the QEMU cpu model to a stripped down z12" - const -> static const for two feature bitmaps David Hildenbrand (14): s390x/kvm: factor out build_channel_report_mcic() into cpu.h s390x/tcg: fix and cleanup mcck injection s390x/tcg: implement SET CLOCK PROGRAMMABLE FIELD s390x/tcg: indicate value of TODPR in STCKE s390x/tcg: wire up STORE CHANNEL REPORT WORD s390x/tcg: ASI/ASGI/ALSI/ALSGI are atomic with Interlocked-acccess facility 1 s390x/tcg: implement Interlocked-Access Facility 2 s390x/tcg: wire up SET ADDRESS LIMIT s390x/tcg: wire up SET CHANNEL MONITOR s390x/tcg: Implement STORE CHANNEL PATH STATUS s390x/tcg: Implement SIGNAL ADAPTER instruction s390x/tcg: implement extract-CPU-time facility s390x/tcg: we already implement the Set-Program-Parameter facility s390x: change the QEMU cpu model to a stripped down z12 hw/s390x/s390-virtio-ccw.c | 8 +++ target/s390x/cpu.h | 26 +++ target/s390x/cpu_models.c | 97 +++--- target/s390x/cpu_models.h | 1 + target/s390x/excp_helper.c | 12 ++-- target/s390x/gen-features.c | 87 +++ target/s390x/helper.h | 6 +- target/s390x/insn-data.def | 29 +--- target/s390x/internal.h | 6 +- target/s390x/kvm.c | 25 +-- target/s390x/misc_helper.c | 59 ++-- target/s390x/translate.c| 164 12 files changed, 415 insertions(+), 105 deletions(-) -- 2.14.3
[Qemu-devel] [PATCH v3 for-2.12 02/14] s390x/tcg: fix and cleanup mcck injection
The architecture mode indication wasn't stored. The split of certain
64bit fields was unnecessary. Also, the complete clock comparator, not
just bit 0-55 (starting at byte 1) was stored.
We now generate a proper MCIC via the same helper we use for KVM.
There is more to clean up, but we will change the other parts later on
either way.
Signed-off-by: David Hildenbrand
---
target/s390x/excp_helper.c | 12 ++--
target/s390x/internal.h| 6 +++---
2 files changed, 9 insertions(+), 9 deletions(-)
diff --git a/target/s390x/excp_helper.c b/target/s390x/excp_helper.c
index d831537544..f4697a884d 100644
--- a/target/s390x/excp_helper.c
+++ b/target/s390x/excp_helper.c
@@ -395,6 +395,9 @@ static void do_mchk_interrupt(CPUS390XState *env)
lowcore = cpu_map_lowcore(env);
+/* we are always in z/Architecture mode */
+lowcore->ar_access_id = 1;
+
for (i = 0; i < 16; i++) {
lowcore->floating_pt_save_area[i] = cpu_to_be64(get_freg(env, i)->ll);
lowcore->gpregs_save_area[i] = cpu_to_be64(env->regs[i]);
@@ -404,13 +407,10 @@ static void do_mchk_interrupt(CPUS390XState *env)
lowcore->prefixreg_save_area = cpu_to_be32(env->psa);
lowcore->fpt_creg_save_area = cpu_to_be32(env->fpc);
lowcore->tod_progreg_save_area = cpu_to_be32(env->todpr);
-lowcore->cpu_timer_save_area[0] = cpu_to_be32(env->cputm >> 32);
-lowcore->cpu_timer_save_area[1] = cpu_to_be32((uint32_t)env->cputm);
-lowcore->clock_comp_save_area[0] = cpu_to_be32(env->ckc >> 32);
-lowcore->clock_comp_save_area[1] = cpu_to_be32((uint32_t)env->ckc);
+lowcore->cpu_timer_save_area = cpu_to_be64(env->cputm);
+lowcore->clock_comp_save_area = cpu_to_be64(env->ckc >> 8);
-lowcore->mcck_interruption_code[0] = cpu_to_be32(0x00400f1d);
-lowcore->mcck_interruption_code[1] = cpu_to_be32(0x4033);
+lowcore->mcic = cpu_to_be64(s390_build_validity_mcic() | MCIC_SC_CP);
lowcore->mcck_old_psw.mask = cpu_to_be64(get_psw_mask(env));
lowcore->mcck_old_psw.addr = cpu_to_be64(env->psw.addr);
mask = be64_to_cpu(lowcore->mcck_new_psw.mask);
diff --git a/target/s390x/internal.h b/target/s390x/internal.h
index 6817b2c432..1a88e4beb4 100644
--- a/target/s390x/internal.h
+++ b/target/s390x/internal.h
@@ -43,7 +43,7 @@ typedef struct LowCore {
uint8_t pad3[0xc8 - 0xc4];/* 0x0c4 */
uint32_tstfl_fac_list;/* 0x0c8 */
uint8_t pad4[0xe8 - 0xcc];/* 0x0cc */
-uint32_tmcck_interruption_code[2]; /* 0x0e8 */
+uint64_tmcic; /* 0x0e8 */
uint8_t pad5[0xf4 - 0xf0];/* 0x0f0 */
uint32_texternal_damage_code; /* 0x0f4 */
uint64_tfailing_storage_address; /* 0x0f8 */
@@ -118,8 +118,8 @@ typedef struct LowCore {
uint32_tfpt_creg_save_area;/* 0x131c */
uint8_t pad16[0x1324 - 0x1320];/* 0x1320 */
uint32_ttod_progreg_save_area; /* 0x1324 */
-uint32_tcpu_timer_save_area[2];/* 0x1328 */
-uint32_tclock_comp_save_area[2]; /* 0x1330 */
+uint64_tcpu_timer_save_area; /* 0x1328 */
+uint64_tclock_comp_save_area; /* 0x1330 */
uint8_t pad17[0x1340 - 0x1338];/* 0x1338 */
uint32_taccess_regs_save_area[16]; /* 0x1340 */
uint64_tcregs_save_area[16]; /* 0x1380 */
--
2.14.3
[Qemu-devel] [PATCH v3 for-2.12 04/14] s390x/tcg: indicate value of TODPR in STCKE
We were not yet using the value of the TOD Programmable Register.
Reviewed-by: Thomas Huth
Signed-off-by: David Hildenbrand
---
target/s390x/translate.c | 5 +
1 file changed, 5 insertions(+)
diff --git a/target/s390x/translate.c b/target/s390x/translate.c
index d13f531c5b..38e1770e5e 100644
--- a/target/s390x/translate.c
+++ b/target/s390x/translate.c
@@ -3897,7 +3897,10 @@ static ExitStatus op_stcke(DisasContext *s, DisasOps *o)
{
TCGv_i64 c1 = tcg_temp_new_i64();
TCGv_i64 c2 = tcg_temp_new_i64();
+TCGv_i64 todpr = tcg_temp_new_i64();
gen_helper_stck(c1, cpu_env);
+/* 16 bit value store in an uint32_t (only valid bits set) */
+tcg_gen_ld32u_i64(todpr, cpu_env, offsetof(CPUS390XState, todpr));
/* Shift the 64-bit value into its place as a zero-extended
104-bit value. Note that "bit positions 64-103 are always
non-zero so that they compare differently to STCK"; we set
@@ -3905,11 +3908,13 @@ static ExitStatus op_stcke(DisasContext *s, DisasOps *o)
tcg_gen_shli_i64(c2, c1, 56);
tcg_gen_shri_i64(c1, c1, 8);
tcg_gen_ori_i64(c2, c2, 0x1);
+tcg_gen_or_i64(c2, c2, todpr);
tcg_gen_qemu_st64(c1, o->in2, get_mem_index(s));
tcg_gen_addi_i64(o->in2, o->in2, 8);
tcg_gen_qemu_st64(c2, o->in2, get_mem_index(s));
tcg_temp_free_i64(c1);
tcg_temp_free_i64(c2);
+tcg_temp_free_i64(todpr);
/* ??? We don't implement clock states. */
gen_op_movi_cc(s, 0);
return NO_EXIT;
--
2.14.3
[Qemu-devel] [PATCH v3 for-2.12 01/14] s390x/kvm: factor out build_channel_report_mcic() into cpu.h
We'll need it later on in two places. Refactor it to just indicate the
validity bits. While at it, introduce a define for the used CR14 bit (we'll
also need later on).
Signed-off-by: David Hildenbrand
---
target/s390x/cpu.h | 23 +++
target/s390x/kvm.c | 25 ++---
2 files changed, 25 insertions(+), 23 deletions(-)
diff --git a/target/s390x/cpu.h b/target/s390x/cpu.h
index 9cfbbbac04..f9d4d62c48 100644
--- a/target/s390x/cpu.h
+++ b/target/s390x/cpu.h
@@ -351,6 +351,9 @@ extern const struct VMStateDescription vmstate_s390_cpu;
#define CR0_CPU_TIMER_SC0x0400ULL
#define CR0_SERVICE_SC 0x0200ULL
+/* Control register 14 bits */
+#define CR14_CHANNEL_REPORT_SC 0x1000ULL
+
/* MMU */
#define MMU_PRIMARY_IDX 0
#define MMU_SECONDARY_IDX 1
@@ -674,6 +677,26 @@ struct sysib_322 {
#define MCIC_VB_CT 0x0002ULL
#define MCIC_VB_CC 0x0001ULL
+static inline uint64_t s390_build_validity_mcic(void)
+{
+uint64_t mcic;
+
+/*
+ * Indicate all validity bits (no damage) only. Other bits have to be
+ * added by the caller. (storage errors, subclasses and subclass modifiers)
+ */
+mcic = MCIC_VB_WP | MCIC_VB_MS | MCIC_VB_PM | MCIC_VB_IA | MCIC_VB_FP |
+ MCIC_VB_GR | MCIC_VB_CR | MCIC_VB_ST | MCIC_VB_AR | MCIC_VB_PR |
+ MCIC_VB_FC | MCIC_VB_CT | MCIC_VB_CC;
+if (s390_has_feat(S390_FEAT_VECTOR)) {
+mcic |= MCIC_VB_VR;
+}
+if (s390_has_feat(S390_FEAT_GUARDED_STORAGE)) {
+mcic |= MCIC_VB_GS;
+}
+return mcic;
+}
+
/* cpu.c */
int s390_get_clock(uint8_t *tod_high, uint64_t *tod_low);
diff --git a/target/s390x/kvm.c b/target/s390x/kvm.c
index 97c45d5537..9b8b59f2a2 100644
--- a/target/s390x/kvm.c
+++ b/target/s390x/kvm.c
@@ -1852,33 +1852,12 @@ void kvm_s390_io_interrupt(uint16_t subchannel_id,
kvm_s390_floating_interrupt(&irq);
}
-static uint64_t build_channel_report_mcic(void)
-{
-uint64_t mcic;
-
-/* subclass: indicate channel report pending */
-mcic = MCIC_SC_CP |
-/* subclass modifiers: none */
-/* storage errors: none */
-/* validity bits: no damage */
-MCIC_VB_WP | MCIC_VB_MS | MCIC_VB_PM | MCIC_VB_IA | MCIC_VB_FP |
-MCIC_VB_GR | MCIC_VB_CR | MCIC_VB_ST | MCIC_VB_AR | MCIC_VB_PR |
-MCIC_VB_FC | MCIC_VB_CT | MCIC_VB_CC;
-if (s390_has_feat(S390_FEAT_VECTOR)) {
-mcic |= MCIC_VB_VR;
-}
-if (s390_has_feat(S390_FEAT_GUARDED_STORAGE)) {
-mcic |= MCIC_VB_GS;
-}
-return mcic;
-}
-
void kvm_s390_crw_mchk(void)
{
struct kvm_s390_irq irq = {
.type = KVM_S390_MCHK,
-.u.mchk.cr14 = 1 << 28,
-.u.mchk.mcic = build_channel_report_mcic(),
+.u.mchk.cr14 = CR14_CHANNEL_REPORT_SC,
+.u.mchk.mcic = s390_build_validity_mcic() | MCIC_SC_CP,
};
kvm_s390_floating_interrupt(&irq);
}
--
2.14.3
[Qemu-devel] [PATCH v2 1/2] virtio-blk: make queue size configurable
Depending on the configuration, it can be beneficial to adjust the virtio-blk
queue size to something other than the current default of 128. Add a new
property to make the queue size configurable.
Signed-off-by: Mark Kanda
Reviewed-by: Karl Heubaum
Reviewed-by: Martin K. Petersen
Reviewed-by: Ameya More
---
hw/block/virtio-blk.c | 10 +-
include/hw/virtio/virtio-blk.h | 1 +
2 files changed, 10 insertions(+), 1 deletion(-)
diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c
index 05d1440..fb59f57 100644
--- a/hw/block/virtio-blk.c
+++ b/hw/block/virtio-blk.c
@@ -928,6 +928,13 @@ static void virtio_blk_device_realize(DeviceState *dev,
Error **errp)
error_setg(errp, "num-queues property must be larger than 0");
return;
}
+if (!is_power_of_2(conf->queue_size) ||
+conf->queue_size > VIRTQUEUE_MAX_SIZE) {
+error_setg(errp, "invalid queue-size property (%" PRIu16 "), "
+ "must be a power of 2 (max %d)",
+ conf->queue_size, VIRTQUEUE_MAX_SIZE);
+return;
+}
blkconf_serial(&conf->conf, &conf->serial);
blkconf_apply_backend_options(&conf->conf,
@@ -953,7 +960,7 @@ static void virtio_blk_device_realize(DeviceState *dev,
Error **errp)
s->sector_mask = (s->conf.conf.logical_block_size / BDRV_SECTOR_SIZE) - 1;
for (i = 0; i < conf->num_queues; i++) {
-virtio_add_queue(vdev, 128, virtio_blk_handle_output);
+virtio_add_queue(vdev, conf->queue_size, virtio_blk_handle_output);
}
virtio_blk_data_plane_create(vdev, conf, &s->dataplane, &err);
if (err != NULL) {
@@ -1012,6 +1019,7 @@ static Property virtio_blk_properties[] = {
DEFINE_PROP_BIT("request-merging", VirtIOBlock, conf.request_merging, 0,
true),
DEFINE_PROP_UINT16("num-queues", VirtIOBlock, conf.num_queues, 1),
+DEFINE_PROP_UINT16("queue-size", VirtIOBlock, conf.queue_size, 128),
DEFINE_PROP_LINK("iothread", VirtIOBlock, conf.iothread, TYPE_IOTHREAD,
IOThread *),
DEFINE_PROP_END_OF_LIST(),
diff --git a/include/hw/virtio/virtio-blk.h b/include/hw/virtio/virtio-blk.h
index d3c8a6f..5117431 100644
--- a/include/hw/virtio/virtio-blk.h
+++ b/include/hw/virtio/virtio-blk.h
@@ -39,6 +39,7 @@ struct VirtIOBlkConf
uint32_t config_wce;
uint32_t request_merging;
uint16_t num_queues;
+uint16_t queue_size;
};
struct VirtIOBlockDataPlane;
--
1.8.3.1
[Qemu-devel] [PATCH v2 2/2] virtio-blk: reject configs with logical block size > physical block size
virtio-blk logical block size should never be larger than physical block
size because it doesn't make sense to have such configurations. QEMU doesn't
have a way to effectively express this condition; the best it can do is
report the physical block exponent as 0 - indicating the logical block size
equals the physical block size.
This is identical to commit 3da023b5827543ee4c022986ea2ad9d1274410b2
but applied to virtio-blk (instead of virtio-scsi).
Signed-off-by: Mark Kanda
Reviewed-by: Konrad Rzeszutek Wilk
Reviewed-by: Ameya More
Reviewed-by: Stefan Hajnoczi
---
hw/block/virtio-blk.c | 7 +++
1 file changed, 7 insertions(+)
diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c
index fb59f57..36c7608 100644
--- a/hw/block/virtio-blk.c
+++ b/hw/block/virtio-blk.c
@@ -952,6 +952,13 @@ static void virtio_blk_device_realize(DeviceState *dev,
Error **errp)
}
blkconf_blocksizes(&conf->conf);
+if (conf->conf.logical_block_size >
+conf->conf.physical_block_size) {
+error_setg(errp,
+ "logical_block_size > physical_block_size not supported");
+return;
+}
+
virtio_init(vdev, "virtio-blk", VIRTIO_ID_BLOCK,
sizeof(struct virtio_blk_config));
--
1.8.3.1
[Qemu-devel] [PATCH v2 0/2] virtio-blk: miscellaneous changes
v2: add check for maximum queue size [Stefan] This series is for two minor virtio-blk changes. The first patch makes the virtio-blk queue size user configurable. The second patch rejects logical block size > physical block configurations (similar to a recent change in virtio-scsi). Mark Kanda (2): virtio-blk: make queue size configurable virtio-blk: reject configs with logical block size > physical block size hw/block/virtio-blk.c | 17 - include/hw/virtio/virtio-blk.h | 1 + 2 files changed, 17 insertions(+), 1 deletion(-) -- 1.8.3.1
[Qemu-devel] [Bug 1737194] [NEW] Windows NT 4.0 fails to boot from qcow2 installation
Public bug reported: Windows NT 4.0 will not boot from an installation more than once if installed in a qcow2 image file. A quick fix to this problem is to use the qcow format instead. Steps to reproduce this issue: Create the image file: qemu-img create -f qcow2 winnt4.qcow2 1G Boot from a Windows NT 4.0 Workstation CD: qemu-system-i386 -hda winnt4.qcow2 -cdrom /dev/cdrom -boot d -m 128 -cpu pentium -vga cirrus During the installation process you have the choise between FAT and NTFS. You can pick anyone. After finishing the installation the guest will reboot to install additional items. Once this is done the guest will be bootable. Eject any CD media from QEMU and reboot. You will then see Windows NT 4.0 booting up to the desktop. Go to "Start->Shut down" to shut down. Then when Windows is ready quit QEMU. Now try to boot using this command: qemu-system-i386 -hda winnt4.qcow2 -boot c -m 128 -cpu pentium -vga cirrus The BIOS screen will display an error message: For NTFS: Booting from Hard Disk... A disk read error occurred. Insert a system diskette and restart the system. For FAT: No bootable device. Additional information: qemu-system-i386 version: 2.10.1 qemu-img version: 2.10.92 (v2.11.0-rc4-dirty) If you don't have a Windows NT 4.0 Workstation installation CD, you may download one from here: https://winworldpc.com/product/windows-nt-40/40 ** Affects: qemu Importance: Undecided Status: New -- You received this bug notification because you are a member of qemu- devel-ml, which is subscribed to QEMU. https://bugs.launchpad.net/bugs/1737194 Title: Windows NT 4.0 fails to boot from qcow2 installation Status in QEMU: New Bug description: Windows NT 4.0 will not boot from an installation more than once if installed in a qcow2 image file. A quick fix to this problem is to use the qcow format instead. Steps to reproduce this issue: Create the image file: qemu-img create -f qcow2 winnt4.qcow2 1G Boot from a Windows NT 4.0 Workstation CD: qemu-system-i386 -hda winnt4.qcow2 -cdrom /dev/cdrom -boot d -m 128 -cpu pentium -vga cirrus During the installation process you have the choise between FAT and NTFS. You can pick anyone. After finishing the installation the guest will reboot to install additional items. Once this is done the guest will be bootable. Eject any CD media from QEMU and reboot. You will then see Windows NT 4.0 booting up to the desktop. Go to "Start->Shut down" to shut down. Then when Windows is ready quit QEMU. Now try to boot using this command: qemu-system-i386 -hda winnt4.qcow2 -boot c -m 128 -cpu pentium -vga cirrus The BIOS screen will display an error message: For NTFS: Booting from Hard Disk... A disk read error occurred. Insert a system diskette and restart the system. For FAT: No bootable device. Additional information: qemu-system-i386 version: 2.10.1 qemu-img version: 2.10.92 (v2.11.0-rc4-dirty) If you don't have a Windows NT 4.0 Workstation installation CD, you may download one from here: https://winworldpc.com/product/windows-nt-40/40 To manage notifications about this bug go to: https://bugs.launchpad.net/qemu/+bug/1737194/+subscriptions
Re: [Qemu-devel] [PATCH v2 0/9] blockdev: fix QMP 'transaction' with IOThreads
On Wed, Dec 06, 2017 at 02:45:41PM +, Stefan Hajnoczi wrote: > v2: > * Use StrOrNull for x-blockdev-set-iothread iothread argument [eblake] > > (This is for QEMU 2.12 because this bug is not -rc4 critical) > > Previously AioContext was held across QMP 'transaction' in an attempt to > achieve bdrv_drained_begin/end() semantics. Nowadays we have > bdrv_drained_begin/end() and the AioContext lock just protects state. > Therefore there is no reason to hold AioContext across > .prepare/.commit/.abort/.clean() anymore. > > Besides being cleanup-worthy, holding AioContext also exposes a bug: > BDRV_POLL_WHILE() doesn't support recursive AioContext locking and will hang > if > depth > 1. This is easy to trigger by submitting a transaction with 2 actions > that touch two drives assigned to an IOThread. The IOThread's AioContext will > be locked twice and BDRV_POLL_WHILE() will hang. > > BDRV_POLL_WHILE() is best fixed by eliminating the AioContext lock entirely in > favor of fine-grained locking. I discussed some fixes for BDRV_POLL_WHILE() > with Paolo but we came to the conclusion that it will just add complexity when > we really want to stop using AioContext locking. > > Summary: > > * Patch 1 fixes missing AioContext lock protection > > * Patches 2-6 clean up excessive AioContext locked regions in QMP >'transaction' to solve the hang > > * Patches 7-9 add a qemu-iotests test case and the necessary infrastructure > > Stefan Hajnoczi (9): > blockdev: hold AioContext for bdrv_unref() in > external_snapshot_clean() > block: don't keep AioContext acquired after > external_snapshot_prepare() > block: don't keep AioContext acquired after drive_backup_prepare() > block: don't keep AioContext acquired after blockdev_backup_prepare() > block: don't keep AioContext acquired after > internal_snapshot_prepare() > block: drop unused BlockDirtyBitmapState->aio_context field > iothread: add iothread_by_id() API > blockdev: add x-blockdev-set-iothread testing command > qemu-iotests: add 202 external snapshots IOThread test > > qapi/block-core.json | 36 +++ > include/sysemu/iothread.h | 1 + > blockdev.c | 258 > + > iothread.c | 7 ++ > tests/qemu-iotests/202 | 95 + > tests/qemu-iotests/202.out | 11 ++ > tests/qemu-iotests/group | 1 + > 7 files changed, 339 insertions(+), 70 deletions(-) > create mode 100755 tests/qemu-iotests/202 > create mode 100644 tests/qemu-iotests/202.out > > -- > 2.14.3 > Thanks, applied to my block-next tree: https://github.com/stefanha/qemu/commits/block-next Stefan signature.asc Description: PGP signature
Re: [Qemu-devel] [PATCH 2/5] lock-guard: add scoped lock implementation
On Fri, Dec 08, 2017 at 11:55:50AM +0100, Paolo Bonzini wrote:
The implementation is somewhat complex. Please structure the header
file so the public interfaces are clear and documented. Move the
implementation out of the way and mark it private. That will make it
easier for someone to figure out "how do I use lock-guard.h".
> diff --git a/include/qemu/lock-guard.h b/include/qemu/lock-guard.h
> new file mode 100644
> index 00..e6a83bf9ee
> --- /dev/null
> +++ b/include/qemu/lock-guard.h
> @@ -0,0 +1,103 @@
> +#ifndef QEMU_LOCK_GUARD_H
> +#define QEMU_LOCK_GUARD_H 1
> +
> +typedef void QemuLockGuardFunc(void *);
> +typedef struct QemuLockGuard {
> +QemuLockGuardFunc *p_lock_fn, *p_unlock_fn;
> +void *lock;
> +int locked;
bool?
> +#define QEMU_WITH_LOCK(type, name, lock) \
> +for (QEMU_LOCK_GUARD(type, name, lock);\
> + qemu_lock_guard_is_taken(&name); \
> + qemu_lock_guard_unlock(&name))
I don't understand the need for the qemu_lock_guard_is_taken(&name)
condition, why not do the following?
for (QEMU_LOCK_GUARD(type, name, lock);
;
qemu_lock_guard_unlock(&name))
Also, the for loop means that break statements do not work inside
QEMU_WITH_LOCK() { ... }. This needs to be documented.
signature.asc
Description: PGP signature
Re: [Qemu-devel] [PATCH v2 3/6] test-bdrv-drain: Test BlockDriver callbacks for drain
On 12/06/2017 04:53 AM, Kevin Wolf wrote:
> This adds a test case that the BlockDriver callbacks for drain are
> called in bdrv_drained_all_begin/end(), and that both of them are called
> exactly once.
>
> Signed-off-by: Kevin Wolf
> ---
> tests/test-bdrv-drain.c | 137
>
> tests/Makefile.include | 2 +
> 2 files changed, 139 insertions(+)
> create mode 100644 tests/test-bdrv-drain.c
>
> +static int coroutine_fn bdrv_test_co_preadv(BlockDriverState *bs,
> +uint64_t offset, uint64_t bytes,
> +QEMUIOVector *qiov, int flags)
> +{
> +/* We want this request to stay until the polling loop in drain waits for
> + * it to complete. We need to sleep a while as bdrv_drain_invoke() comes
> + * first and polls its result, too, but it shouldn't accidentally
> complete
> + * this request yet. */
> +co_aio_sleep_ns(qemu_get_aio_context(), QEMU_CLOCK_REALTIME, 10);
> +
Conflicts with patches to remove the first parameter from co_aio_sleep_ns():
https://lists.gnu.org/archive/html/qemu-devel/2017-11/msg01681.html
Should be an obvious fix for whoever rebases on top of the other, so:
Reviewed-by: Eric Blake
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3266
Virtualization: qemu.org | libvirt.org
signature.asc
Description: OpenPGP digital signature
Re: [Qemu-devel] [PATCH 5/5] thread-pool: convert to use lock guards
On Fri, Dec 08, 2017 at 11:55:53AM +0100, Paolo Bonzini wrote:
> @@ -88,9 +88,9 @@ static void *worker_thread(void *opaque)
>
> do {
> pool->idle_threads++;
> -qemu_mutex_unlock(&pool->lock);
> +qemu_lock_guard_unlock(&pool_guard);
> ret = qemu_sem_timedwait(&pool->sem, 1);
> -qemu_mutex_lock(&pool->lock);
> +qemu_lock_guard_lock(&pool_guard);
Or:
QEMU_WITHOUT_LOCK_GUARD(pool_guard) {
ret = qemu_sem_timedwait(&pool->sem, 1);
}
Seems funny but I like it. :)
Unfortunately it's tricky to come up with good semantics. A 'return'
statement inside QEMU_WITHOUT_LOCK_GUARD() should leave the lock
unlocked. But a 'break' statement may need to reacquire the lock...
> @@ -258,12 +254,12 @@ BlockAIOCB *thread_pool_submit_aio(ThreadPool *pool,
>
> trace_thread_pool_submit(pool, req, arg);
>
> -qemu_mutex_lock(&pool->lock);
> +QEMU_LOCK_GUARD(QemuMutex, pool_guard, &pool->lock);
> if (pool->idle_threads == 0 && pool->cur_threads < pool->max_threads) {
> spawn_thread(pool);
> }
> QTAILQ_INSERT_TAIL(&pool->request_list, req, reqs);
> -qemu_mutex_unlock(&pool->lock);
> +qemu_lock_guard_unlock(&pool_guard);
Why not QEMU_WITH_LOCK()? Then you can get rid of the explicit unlock.
> @@ -330,7 +326,7 @@ void thread_pool_free(ThreadPool *pool)
>
> assert(QLIST_EMPTY(&pool->head));
>
> -qemu_mutex_lock(&pool->lock);
> +QEMU_LOCK_GUARD(QemuMutex, pool_guard, &pool->lock);
>
> /* Stop new threads from spawning */
> qemu_bh_delete(pool->new_thread_bh);
> @@ -344,7 +340,7 @@ void thread_pool_free(ThreadPool *pool)
> qemu_cond_wait(&pool->worker_stopped, &pool->lock);
> }
>
> -qemu_mutex_unlock(&pool->lock);
> +qemu_lock_guard_unlock(&pool_guard);
Here too. I don't see the advantage of replacing a single lock/unlock
with QEMU_LOCK_GUARD/unlock, if it cannot be made shorter/safer then
it's fine to use QemuMutex directly.
signature.asc
Description: PGP signature
[Qemu-devel] [PATCH 1/3] hw/arm/virt: Add virt-2.12 machine type
Add virt-2.12 machine type.
Signed-off-by: Peter Maydell
---
include/hw/compat.h | 3 +++
hw/arm/virt.c | 19 +--
2 files changed, 20 insertions(+), 2 deletions(-)
diff --git a/include/hw/compat.h b/include/hw/compat.h
index cf389b4..263de97 100644
--- a/include/hw/compat.h
+++ b/include/hw/compat.h
@@ -1,6 +1,9 @@
#ifndef HW_COMPAT_H
#define HW_COMPAT_H
+#define HW_COMPAT_2_11 \
+/* empty */
+
#define HW_COMPAT_2_10 \
{\
.driver = "virtio-mouse-device",\
diff --git a/hw/arm/virt.c b/hw/arm/virt.c
index 151592b..543f9bd 100644
--- a/hw/arm/virt.c
+++ b/hw/arm/virt.c
@@ -1618,7 +1618,7 @@ static void machvirt_machine_init(void)
}
type_init(machvirt_machine_init);
-static void virt_2_11_instance_init(Object *obj)
+static void virt_2_12_instance_init(Object *obj)
{
VirtMachineState *vms = VIRT_MACHINE(obj);
VirtMachineClass *vmc = VIRT_MACHINE_GET_CLASS(vms);
@@ -1678,10 +1678,25 @@ static void virt_2_11_instance_init(Object *obj)
vms->irqmap = a15irqmap;
}
+static void virt_machine_2_12_options(MachineClass *mc)
+{
+}
+DEFINE_VIRT_MACHINE_AS_LATEST(2, 12)
+
+#define VIRT_COMPAT_2_11 \
+HW_COMPAT_2_11
+
+static void virt_2_11_instance_init(Object *obj)
+{
+virt_2_12_instance_init(obj);
+}
+
static void virt_machine_2_11_options(MachineClass *mc)
{
+virt_machine_2_12_options(mc);
+SET_MACHINE_COMPAT(mc, VIRT_COMPAT_2_11);
}
-DEFINE_VIRT_MACHINE_AS_LATEST(2, 11)
+DEFINE_VIRT_MACHINE(2, 11)
#define VIRT_COMPAT_2_10 \
HW_COMPAT_2_10
--
2.7.4
Re: [Qemu-devel] [PATCH v2 for-2.12 08/10] s390x/tcg: implement extract-CPU-time facility
On 12/08/2017 07:02 AM, David Hildenbrand wrote:
> +/* Store CPU Timer (also used for EXTRACT CPU TIME) */
> +uint64_t HELPER(stpt)(CPUS390XState *env)
> +{
> +#if defined(CONFIG_USER_ONLY)
> +/*
> + * Fake a descending CPU timer. We could get negative values here,
> + * but we don't care as it is up to the OS when to process that
> + * interrupt and reset to > 0.
> + */
> +return UINT64_MAX - (uint64_t)cpu_get_host_ticks();
> +#else
> +return time2tod(env->cputm - qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL));
> +#endif
> +}
> +
>
Looks good to me.
r~
