Re: [Qemu-devel] [PATCH 2/4] hw/smbios: Restrict access to "smbios_ipmi.h"
On 12/07/18 17:51, Philippe Mathieu-Daudé wrote:
> All the consumers of "smbios_ipmi.h" are located in hw/smbios/.
I tried to verify this statement by grepping the tree for
"smbios_ipmi.h". There were zero hits. Please use the more precise
pathname "hw/smbios/ipmi.h". (I can't suggest just "ipmi.h", because
that isn't unique.)
With this update:
Reviewed-by: Laszlo Ersek
Thanks,
Laszlo
> There is no need to have this include publicly exposed,
> reduce the visibility by moving it in hw/smbios/.
>
> Signed-off-by: Philippe Mathieu-Daudé
> ---
> hw/smbios/smbios.c | 2 +-
> include/hw/smbios/ipmi.h => hw/smbios/smbios_ipmi.h | 0
> hw/smbios/smbios_type_38-stub.c | 2 +-
> hw/smbios/smbios_type_38.c | 2 +-
> 4 files changed, 3 insertions(+), 3 deletions(-)
> rename include/hw/smbios/ipmi.h => hw/smbios/smbios_ipmi.h (100%)
>
> diff --git a/hw/smbios/smbios.c b/hw/smbios/smbios.c
> index 920939454e..30bd4731cf 100644
> --- a/hw/smbios/smbios.c
> +++ b/hw/smbios/smbios.c
> @@ -28,7 +28,7 @@
> #include "hw/loader.h"
> #include "exec/cpu-common.h"
> #include "smbios_build.h"
> -#include "hw/smbios/ipmi.h"
> +#include "smbios_ipmi.h"
>
> /* legacy structures and constants for <= 2.0 machines */
> struct smbios_header {
> diff --git a/include/hw/smbios/ipmi.h b/hw/smbios/smbios_ipmi.h
> similarity index 100%
> rename from include/hw/smbios/ipmi.h
> rename to hw/smbios/smbios_ipmi.h
> diff --git a/hw/smbios/smbios_type_38-stub.c b/hw/smbios/smbios_type_38-stub.c
> index 5b83c9b1f1..fc4516bc8a 100644
> --- a/hw/smbios/smbios_type_38-stub.c
> +++ b/hw/smbios/smbios_type_38-stub.c
> @@ -8,7 +8,7 @@
> */
>
> #include "qemu/osdep.h"
> -#include "hw/smbios/ipmi.h"
> +#include "smbios_ipmi.h"
>
> void smbios_build_type_38_table(void)
> {
> diff --git a/hw/smbios/smbios_type_38.c b/hw/smbios/smbios_type_38.c
> index 56e8609c00..d84e87d608 100644
> --- a/hw/smbios/smbios_type_38.c
> +++ b/hw/smbios/smbios_type_38.c
> @@ -9,10 +9,10 @@
>
> #include "qemu/osdep.h"
> #include "hw/ipmi/ipmi.h"
> -#include "hw/smbios/ipmi.h"
> #include "hw/smbios/smbios.h"
> #include "qemu/error-report.h"
> #include "smbios_build.h"
> +#include "smbios_ipmi.h"
>
> /* SMBIOS type 38 - IPMI */
> struct smbios_type_38 {
>
Re: [Qemu-devel] [PATCH] hw/s390/ccw.c: Don't take address of packed members
On 12/10/2018 08:58 AM, Peter Maydell wrote:
Taking the address of a field in a packed struct is a bad idea, because
it might not be actually aligned enough for that pointer type (and
thus cause a crash on dereference on some host architectures). Newer
versions of clang warn about this.
Avoid the problem by using local copies of the PMCW and SCSW
struct fields in copy_schib_from_guest() and copy_schib_to_guest().
Signed-off-by: Peter Maydell
---
This seemed like a not totally ugly and reasonably localised fix
that satisfies clang. Oddly, this makes the generated object file
15K smaller (421K vs 406K), so it might even be better code...
hw/s390x/css.c | 20
1 file changed, 16 insertions(+), 4 deletions(-)
diff --git a/hw/s390x/css.c b/hw/s390x/css.c
index 04ec5cc9705..ef07691e36b 100644
--- a/hw/s390x/css.c
+++ b/hw/s390x/css.c
@@ -1290,9 +1290,15 @@ void copy_scsw_to_guest(SCSW *dest, const SCSW *src)
static void copy_schib_to_guest(SCHIB *dest, const SCHIB *src)
{
int i;
+PMCW srcpmcw, destpmcw;
+SCSW srcscsw, destscsw;
-copy_pmcw_to_guest(&dest->pmcw, &src->pmcw);
-copy_scsw_to_guest(&dest->scsw, &src->scsw);
+srcpmcw = src->pmcw;
+copy_pmcw_to_guest(&destpmcw, &srcpmcw);
+dest->pmcw = destpmcw;
+srcscsw = src->scsw;
+copy_scsw_to_guest(&destscsw, &srcscsw);
+dest->scsw = destscsw;
dest->mba = cpu_to_be64(src->mba);
for (i = 0; i < ARRAY_SIZE(dest->mda); i++) {
dest->mda[i] = src->mda[i];
@@ -1339,9 +1345,15 @@ static void copy_scsw_from_guest(SCSW *dest, const SCSW
*src)
static void copy_schib_from_guest(SCHIB *dest, const SCHIB *src)
{
int i;
+PMCW srcpmcw, destpmcw;
+SCSW srcscsw, destscsw;
-copy_pmcw_from_guest(&dest->pmcw, &src->pmcw);
-copy_scsw_from_guest(&dest->scsw, &src->scsw);
+srcpmcw = src->pmcw;
+copy_pmcw_from_guest(&destpmcw, &srcpmcw);
+dest->pmcw = destpmcw;
+srcscsw = src->scsw;
+copy_scsw_from_guest(&destscsw, &srcscsw);
+dest->scsw = destscsw;
dest->mba = be64_to_cpu(src->mba);
for (i = 0; i < ARRAY_SIZE(dest->mda); i++) {
dest->mda[i] = src->mda[i];
Reviewed-by: Farhan Ali
[Qemu-devel] Fwd: [PATCH] QEMU patch for PCI handling bug (invalid free)
Am 10.12.2018 um 14:38 hat Matthias Weckbecker geschrieben: > Hi Kevin, > > I'm attaching a patch for qemu. Read below for details. > > There's a bug in qemu in the PCI bridge handling that can be triggered when > following the steps below: > > 1) Create some VM (e.g. w/ virsh define) > 2) Ensure there is a PCI bridge attached, e.g. w/ libvirt like this: > > > > > >function='0x1'/> > > > 3) Take a snapshot while it's *running*, like this: > > % virsh start mips64el-vm > % virsh snapshot-create-as mips64el-vm mips64el-snap > > 4) Destroy the VM and revert from snapshot: > > % virsh destroy mips64el-vm > % virsh snapshot-revert mips64el-vm mips64el-snap --running >error: internal error: process exited while connecting to monitor: > corrupted size vs. prev_size > > 5) qemu-system-mips64el crashes > > The attached patch resolves the issue. Can you maybe review+commit, please? Hi Matthias, thanks for sending the patch. The next step is that it needs to be reviewed on the qemu-devel mailing list (CCed) and then the PCI subsystem maintainers (Michael and Marcel, CCed as well) can commit it. Maybe some of the explanation above should actually be moved to the commit message of the patch itself? Kevin - Forwarded message from Matthias Weckbecker - (gdb) bt #0 __GI_raise (sig=sig@entry=6) at ../sysdeps/unix/sysv/linux/raise.c:51 #1 0x7fde12dfc801 in __GI_abort () at abort.c:79 #2 0x7fde12e45897 in __libc_message (action=action@entry=do_abort, fmt=fmt@entry=0x7fde12f72b9a "%s\n") at ../sysdeps/posix/libc_fatal.c:181 #3 0x7fde12e4c90a in malloc_printerr (str=str@entry=0x7fde12f70c9d "corrupted size vs. prev_size") at malloc.c:5350 #4 0x7fde12e4cb0c in malloc_consolidate (av=av@entry=0x7fde131a7c40 ) at malloc.c:4456 #5 0x7fde12e5403b in _int_free (have_lock=0, p=, av=0x7fde131a7c40 ) at malloc.c:4362 #6 __GI___libc_free (mem=0x55f089173c20) at malloc.c:3124 #7 0x55f086c85062 in phys_section_destroy (mr=0x55f089173c20) at ./exec.c:1317 #8 phys_sections_free (map=0x55f0890f1560) at ./exec.c:1325 #9 address_space_dispatch_free (d=0x55f0890f1550) at ./exec.c:2777 #10 0x55f086cc0509 in flatview_destroy (view=0x55f088a5caf0) at ./memory.c:301 #11 0x55f087031dc0 in call_rcu_thread (opaque=) at ./util/rcu.c:272 #12 0x7fde131b46db in start_thread (arg=0x7fde0aa39700) at pthread_create.c:463 #13 0x7fde12edd88f in clone () at ../sysdeps/unix/sysv/linux/x86_64/clone.S:95 ==13641== Thread 2: [0/5041] ==13641== Invalid read of size 8 ==13641==at 0x42755B: memory_region_unref (memory.c:1749) ==13641==by 0x42755B: flatview_destroy (memory.c:307) ==13641==by 0x798DBF: call_rcu_thread (rcu.c:272) ==13641==by 0x97BF6DA: start_thread (pthread_create.c:463) ==13641==by 0x9AF888E: clone (clone.S:95) ==13641== Address 0x408e4670 is 64 bytes inside a block of size 1,440 free'd ==13641==at 0x4C30D3B: free (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so) ==13641==by 0x5E988B: get_pci_config_device (pci.c:491) ==13641==by 0x660069: vmstate_load_state (vmstate.c:140) ==13641==by 0x660236: vmstate_load_state (vmstate.c:137) ==13641==by 0x659994: vmstate_load (savevm.c:748) ==13641==by 0x65A7B1: qemu_loadvm_section_start_full.isra.11 (savevm.c:1918) ==13641==by 0x65AA67: qemu_loadvm_state_main.isra.13 (savevm.c:2013) ==13641==by 0x65D7CE: qemu_loadvm_state (savevm.c:2092) ==13641==by 0x65E40D: load_snapshot (savevm.c:2406) ==13641==by 0x3E28C2: main (vl.c:4918) ==13641== Block was alloc'd at ==13641==at 0x4C2FB0F: malloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so) ==13641==by 0x8D35A28: g_malloc (in /usr/lib/x86_64-linux-gnu/libglib-2.0.so.0.5600.3) ==13641==by 0x5ECA8A: pci_bridge_region_init (pci_bridge.c:187) ==13641==by 0x5ECEFC: pci_bridge_initfn (pci_bridge.c:384) ==13641==by 0x5E654F: pci_bridge_dev_realize (pci_bridge_dev.c:59) ==13641==by 0x5EBED0: pci_qdev_realize (pci.c:2034) ==13641==by 0x5742D8: device_set_realized (qdev.c:914) ==13641==by 0x6B8F96: property_set_bool (object.c:1906) ==13641==by 0x6BD11E: object_property_set_qobject (qom-qobject.c:27) ==13641==by 0x6BAD7F: object_property_set_bool (object.c:1171) ==13641==by 0x4FA75D: qdev_device_add (qdev-monitor.c:629) ==13641==by 0x4FCD36: device_init_func (vl.c:2432) ==13641== >From 8229eb9cb97a1806e264290e2935261bf23c7f34 Mon Sep 17 00:00:00 2001 From: Matthias Weckbecker Date: Mon, 10 Dec 2018 14:00:48 +0100 Subject: [PATCH] hw/pci-bridge: Fix invalid free() When loadvm'ing a *running* snapshot qemu crashes due to an invalid free. It's fortunately caught early by glibc heap memory corruption protection and
Re: [Qemu-devel] [PATCH 1/4] tests: Remove unused include
On 12/07/18 17:51, Philippe Mathieu-Daudé wrote: > Signed-off-by: Philippe Mathieu-Daudé > --- > tests/acpi-utils.c | 1 - > 1 file changed, 1 deletion(-) > > diff --git a/tests/acpi-utils.c b/tests/acpi-utils.c > index 41dc1ea9b4..297af55d39 100644 > --- a/tests/acpi-utils.c > +++ b/tests/acpi-utils.c > @@ -15,7 +15,6 @@ > #include "qemu/osdep.h" > #include > #include "qemu-common.h" > -#include "hw/smbios/smbios.h" > #include "qemu/bitmap.h" > #include "acpi-utils.h" > #include "boot-sector.h" > Opinions vary whether empty commit message bodies are good style or not. Personally I prefer to put at least one sentence in there, even if it only repeats the subject line. Up to subsystem maintainers to decide I guess. With the commit message updated, or not: Reviewed-by: Laszlo Ersek
Re: [Qemu-devel] [PATCH v3 1/3] target/arm: Introduce arm_hcr_el2_eff
On 12/10/18 8:22 AM, Peter Maydell wrote:
> This section that clears VI/VF/VSE is new, and I'm not sure it's right.
> The spec says that the virtual IRQ interrupt is enabled only if {TGE,IMO}
> is {0,1}, but the meaning of the bit is "pending", and an interrupt
> can be pending without being enabled. Ditto VF, VSE.
Fair enough. I can re-send with this section removed.
r~
Re: [Qemu-devel] [PATCH] cpus.c: Fix race condition in cpu_stop_current()
Le 12/10/18 à 3:39 PM, Peter Maydell a écrit : On Mon, 10 Dec 2018 at 14:30, KONRAD Frederic wrote: Hi Peter, Thanks for that patch! I'm seeing the same kind of issue when I run 8 qemu in parallel but it doesn't seem to be fixed by this patch. Is it supposed to fix the issue when we are doing a reset_request through a MMIO device? It happens (rarely) with this kind of guest code: exit: write to the register to reset the device loop: branch loop The code after the reset is executed.. can't we exit the loop directly with cpu_loop_exit after cpu_exit? cpu_loop_exit would abort the execution of the store instruction that writes to the reset register. I'm not sure that's a great idea. My thought was more that we should just make sure that insn is the last one in the TB, so effectively we execute that insn and then reset the system before executing any further insns. Thinking it over though I'm not sure that we do do anything that could avoid having more insns following in the same TB, unless you're using singlestep or icount... Exactly I think we don't do anything for that.. But we can't guess which IO will require the loop to be exited though.. Fred thanks -- PMM
Re: [Qemu-devel] [PATCH v3 5/5] crypto: support multiple threads accessing one QCryptoBlock
On Mon, Dec 10, 2018 at 03:06:59PM +0100, Alberto Garcia wrote:
> On Fri 07 Dec 2018 05:13:51 PM CET, Vladimir Sementsov-Ogievskiy wrote:
> > @@ -148,12 +154,97 @@ int qcrypto_block_encrypt(QCryptoBlock *block,
> >
> > QCryptoCipher *qcrypto_block_get_cipher(QCryptoBlock *block)
> > {
> > -return block->cipher;
> > +/* Ciphers should be accessed through pop/push method to be
> > thread-safe.
> > + * Better, they should not be accessed externally at all (note, that
> > + * pop/push are static functions)
> > + * This function is used only in test with one thread (it's safe to
> > skip
> > + * pop/push interface), so it's enough to assert it here:
> > + */
> > +assert(block->n_ciphers <= 1);
> > +return block->ciphers ? block->ciphers[0] : NULL;
>
> If this is only supposed to be called in test mode I think you can also
> assert that g_test_initialized() is true.
>
> And the same with qcrypto_block_get_ivgen() later in this patch.
I consider these APIs as being valid for use anywhere - it just
happens it is only used in the tests right now. So I think it is
ok to assert on n_cipers here.
> > +int qcrypto_block_init_cipher(QCryptoBlock *block,
> > + QCryptoCipherAlgorithm alg,
> > + QCryptoCipherMode mode,
> > + const uint8_t *key, size_t nkey,
> > + size_t n_threads, Error **errp)
> > +{
> > +size_t i;
> > +
> > +assert(!block->ciphers && !block->n_ciphers && !block->n_free_ciphers);
> > +
> > +block->ciphers = g_new0(QCryptoCipher *, n_threads);
>
> You can use g_new() instead of g_new0() because you're anyway
> overwriting all elements of the array.
I'd rather have it initialized to zero upfront, so if creating any
cipher in the array fails, we don't have uninitialized array elements
during later cleanup code.
> But these are minor nits, the patchs looks good to me else.
>
> Reviewed-by: Alberto Garcia
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 v3 3/3] target/arm: Implement the ARMv8.1-LOR extension
On Thu, 6 Dec 2018 at 17:55, Richard Henderson
wrote:
>
> Provide a trivial implementation with zero limited ordering regions,
> which causes the LDLAR and STLLR instructions to devolve into the
> LDAR and STLR instructions from the base ARMv8.0 instruction set.
>
> Signed-off-by: Richard Henderson
>
> ---
> v2: Mark LORID_EL1 read-only.
> Add TLOR access checks.
> Conditionally unmask TLOR in hcr/scr_write.
> v3: Fix isar_feature_aa64_lor.
> Split out access_lor_ns.
> Defer all {E2H,TGE} vs TLOR testing to arm_hcr_el2_eff.
Reviewed-by: Peter Maydell
thanks
-- PMM
Re: [Qemu-devel] [PATCH v3 2/3] target/arm: Use arm_hcr_el2_eff more places
On Thu, 6 Dec 2018 at 17:55, Richard Henderson wrote: > > Since arm_hcr_el2_eff includes a check against > arm_is_secure_below_el3, we can often remove a > nearby check against secure state. > > In some cases, sort the call to arm_hcr_el2_eff > to the end of a short-circuit logical sequence. > > Signed-off-by: Richard Henderson > --- > v3: Do not change regime_translation_disabled. > --- Reviewed-by: Peter Maydell thanks -- PMM
Re: [Qemu-devel] [PATCH] cpus.c: Fix race condition in cpu_stop_current()
On Mon, 10 Dec 2018 at 14:30, KONRAD Frederic wrote: > > Hi Peter, > > Thanks for that patch! > > I'm seeing the same kind of issue when I run 8 qemu in parallel but it doesn't > seem to be fixed by this patch. Is it supposed to fix the issue when we are > doing a reset_request through a MMIO device? > > It happens (rarely) with this kind of guest code: > > exit: >write to the register to reset the device > loop: >branch loop > > The code after the reset is executed.. can't we exit the loop directly with > cpu_loop_exit after cpu_exit? cpu_loop_exit would abort the execution of the store instruction that writes to the reset register. I'm not sure that's a great idea. My thought was more that we should just make sure that insn is the last one in the TB, so effectively we execute that insn and then reset the system before executing any further insns. Thinking it over though I'm not sure that we do do anything that could avoid having more insns following in the same TB, unless you're using singlestep or icount... thanks -- PMM
[Qemu-devel] [TCG SIMD] 128 bit SIMD support in the TCG backend
Hi, Hope this is the right place for this question. I was wondering whether the TCG i386 backend is currently able to generate SIMD code ( vector loads/store, moves, vector arithmetic, etc ). Looking around on patchwork I found these patches: https://patchwork.kernel.org/project/qemu-devel/list/?submitter=172151 They seem to implement the vector support that I'd be interested in (including support for tcg_gen_qemu__v128 ) but I guess the patches didn't make it into any stable version? Any info would be appreciated. Thanks.
Re: [Qemu-devel] [PATCH v2 0/3] fw_cfg: fix boot bootsplash and reboot-timeout error checking
On Tue, Nov 20, 2018 at 09:10:23PM -0800, Li Qiang wrote: > And also do some code cleanup. > A lot of thanks to Markus's review and advice. > > v2: fix some small issue per Markus's review. > > Li Qiang (3): > fw_cfg: fix -boot bootsplash error checking > fw_cfg: fix -boot reboot-timeout error checking > fw_cfg: make qemu_extra_params_fw locally Series is Reviewed-by: Gerd Hoffmann
Re: [Qemu-devel] [PATCH for-4.0] ui/console: Remove qemu_create_display_surface_guestmem()
On Thu, Nov 22, 2018 at 05:03:09PM +, Peter Maydell wrote: > The qemu_create_display_surface_guestmem() function was added in > commit a77549b3ffcc24c32ee4e but apparently never used. Remove it. > > (The API of this function is in any case awkward as a generic > function: it assumes that a physical address uniquely identifies > a piece of memory in the system, which is mostly but not > always true.) Added to ui queue. thanks, Gerd
Re: [Qemu-devel] [PATCH] cpus.c: Fix race condition in cpu_stop_current()
Hi Peter,
Thanks for that patch!
I'm seeing the same kind of issue when I run 8 qemu in parallel but it doesn't
seem to be fixed by this patch. Is it supposed to fix the issue when we are
doing a reset_request through a MMIO device?
It happens (rarely) with this kind of guest code:
exit:
write to the register to reset the device
loop:
branch loop
The code after the reset is executed.. can't we exit the loop directly with
cpu_loop_exit after cpu_exit?
Thanks,
Fred
Le 12/7/18 à 4:59 PM, Peter Maydell a écrit :
We use cpu_stop_current() to ensure the current CPU has stopped
from places like qemu_system_reset_request(). Unfortunately its
current implementation has a race. It calls qemu_cpu_stop(),
which sets cpu->stopped to true even though the CPU hasn't
actually stopped yet. The main thread will look at the flags
set by qemu_system_reset_request() and call pause_all_vcpus().
pause_all_vcpus() waits for every cpu to have cpu->stopped true,
so it can continue (and we will start the system reset operation)
before the vcpu thread has got back to its top level loop.
Instead, just set cpu->stop and call cpu_exit(). This will
cause the vcpu to exit back to the top level loop, and there
(as part of the wait_io_event code) it will call qemu_cpu_stop().
This fixes bugs where the reset request appeared to be ignored
or the CPU misbehaved because the reset operation started
to change vcpu state while the vcpu thread was still using it.
Signed-off-by: Peter Maydell
---
We discussed this a little while back:
https://lists.gnu.org/archive/html/qemu-devel/2018-10/msg00154.html
and Jaap reported a bug which I suspect of being the same thing:
https://lists.gnu.org/archive/html/qemu-discuss/2018-10/msg00014.html
Annoyingly I have lost the test case that demonstrated this
race, but I analysed it at the time and this should definitely
fix it. I have opted not to try to address any of the other
possible cleanup here (eg vm_stop() has a potential similar
race if called from a vcpu thread I suspect), since it gets
pretty tangled.
Jaap: could you test whether this patch fixes the issue you
were seeing, please?
---
cpus.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/cpus.c b/cpus.c
index 0ddeeefc14f..b09b7027126 100644
--- a/cpus.c
+++ b/cpus.c
@@ -2100,7 +2100,8 @@ void qemu_init_vcpu(CPUState *cpu)
void cpu_stop_current(void)
{
if (current_cpu) {
-qemu_cpu_stop(current_cpu, true);
+current_cpu->stop = true;
+cpu_exit(current_cpu);
}
}
Re: [Qemu-devel] [PATCH for-3.2 01/11] vhost-user: define conventions for vhost-user backends
On Mon, Nov 26, 2018 at 04:42:40PM +0400, Marc-André Lureau wrote: > As discussed during "[PATCH v4 00/29] vhost-user for input & GPU" > review, let's define a common set of backend conventions to help with > management layer implementation, and interoperability. > > Signed-off-by: Marc-André Lureau > Reviewed-by: Daniel P. Berrangé Acked-by: Gerd Hoffmann btw: have you seen the idea to use a vfio-style interface for communication between qemu and external device emulation processes? cheers, Gerd
Re: [Qemu-devel] [PATCH for-3.2 10/11] Add vhost-user-input-pci
On Mon, Nov 26, 2018 at 04:42:49PM +0400, Marc-André Lureau wrote:
> Add a new virtio-input device, which connects to a vhost-user
> backend. Usage:
>
> -object vhost-user-backend,id=vuid,chardev=... \
> -device vhost-user-input-pci,vhost-user=vuid
>
> vhost-user-input is similar to virtio-input-host, it is wrapped by
> vhost-user-input-pci. Instead of reading configuration directly from
> an input device / evdev, it reads it over vhost-user protocol with
> INPUT_GET_CONFIG message. Then vhost-user-backend takes care of
> interfacing the virtio device with the backend, for the queue & events
> processing.
Reviewed-by: Gerd Hoffmann
>
> Signed-off-by: Marc-André Lureau
> ---
> hw/virtio/virtio-pci.h | 10 +++
> include/hw/virtio/virtio-input.h | 14
> hw/input/vhost-user-input.c | 110 +++
> hw/virtio/virtio-pci.c | 22 +++
> MAINTAINERS | 1 +
> hw/input/Makefile.objs | 1 +
> 6 files changed, 158 insertions(+)
> create mode 100644 hw/input/vhost-user-input.c
>
> diff --git a/hw/virtio/virtio-pci.h b/hw/virtio/virtio-pci.h
> index 813082b0d7..c7e28e1b9c 100644
> --- a/hw/virtio/virtio-pci.h
> +++ b/hw/virtio/virtio-pci.h
> @@ -54,6 +54,7 @@ typedef struct VirtIORngPCI VirtIORngPCI;
> typedef struct VirtIOInputPCI VirtIOInputPCI;
> typedef struct VirtIOInputHIDPCI VirtIOInputHIDPCI;
> typedef struct VirtIOInputHostPCI VirtIOInputHostPCI;
> +typedef struct VHostUserInputPCI VHostUserInputPCI;
> typedef struct VirtIOGPUPCI VirtIOGPUPCI;
> typedef struct VHostVSockPCI VHostVSockPCI;
> typedef struct VirtIOCryptoPCI VirtIOCryptoPCI;
> @@ -376,6 +377,15 @@ struct VirtIOInputHostPCI {
>
> #endif
>
> +#define TYPE_VHOST_USER_INPUT_PCI "vhost-user-input-pci"
> +#define VHOST_USER_INPUT_PCI(obj)\
> +OBJECT_CHECK(VHostUserInputPCI, (obj), TYPE_VHOST_USER_INPUT_PCI)
> +
> +struct VHostUserInputPCI {
> +VirtIOPCIProxy parent_obj;
> +VHostUserInput vhi;
> +};
> +
> /*
> * virtio-gpu-pci: This extends VirtioPCIProxy.
> */
> diff --git a/include/hw/virtio/virtio-input.h
> b/include/hw/virtio/virtio-input.h
> index 054c38836f..4fca03e796 100644
> --- a/include/hw/virtio/virtio-input.h
> +++ b/include/hw/virtio/virtio-input.h
> @@ -2,6 +2,7 @@
> #define QEMU_VIRTIO_INPUT_H
>
> #include "ui/input.h"
> +#include "sysemu/vhost-user-backend.h"
>
> /* - */
> /* virtio input protocol */
> @@ -42,11 +43,18 @@ typedef struct virtio_input_event virtio_input_event;
> #define VIRTIO_INPUT_HOST_GET_PARENT_CLASS(obj) \
> OBJECT_GET_PARENT_CLASS(obj, TYPE_VIRTIO_INPUT_HOST)
>
> +#define TYPE_VHOST_USER_INPUT "vhost-user-input"
> +#define VHOST_USER_INPUT(obj) \
> +OBJECT_CHECK(VHostUserInput, (obj), TYPE_VHOST_USER_INPUT)
> +#define VHOST_USER_INPUT_GET_PARENT_CLASS(obj) \
> +OBJECT_GET_PARENT_CLASS(obj, TYPE_VHOST_USER_INPUT)
> +
> typedef struct VirtIOInput VirtIOInput;
> typedef struct VirtIOInputClass VirtIOInputClass;
> typedef struct VirtIOInputConfig VirtIOInputConfig;
> typedef struct VirtIOInputHID VirtIOInputHID;
> typedef struct VirtIOInputHost VirtIOInputHost;
> +typedef struct VHostUserInput VHostUserInput;
>
> struct VirtIOInputConfig {
> virtio_input_config config;
> @@ -98,6 +106,12 @@ struct VirtIOInputHost {
> int fd;
> };
>
> +struct VHostUserInput {
> +VirtIOInput parent_obj;
> +
> +VhostUserBackend *vhost;
> +};
> +
> void virtio_input_send(VirtIOInput *vinput, virtio_input_event *event);
> void virtio_input_init_config(VirtIOInput *vinput,
>virtio_input_config *config);
> diff --git a/hw/input/vhost-user-input.c b/hw/input/vhost-user-input.c
> new file mode 100644
> index 00..ef1b23a8b2
> --- /dev/null
> +++ b/hw/input/vhost-user-input.c
> @@ -0,0 +1,110 @@
> +/*
> + * This work is licensed under the terms of the GNU GPL, version 2 or
> + * (at your option) any later version. See the COPYING file in the
> + * top-level directory.
> + */
> +
> +#include "qemu/osdep.h"
> +#include "qapi/error.h"
> +#include "qemu-common.h"
> +
> +#include "hw/qdev.h"
> +#include "hw/virtio/virtio-input.h"
> +
> +static void vhost_input_realize(DeviceState *dev, Error **errp)
> +{
> +VHostUserInput *vhi = VHOST_USER_INPUT(dev);
> +VirtIOInput *vinput = VIRTIO_INPUT(dev);
> +VirtIODevice *vdev = VIRTIO_DEVICE(dev);
> +virtio_input_config *config;
> +int i, ret;
> +
> +if (!vhi->vhost) {
> +error_setg(errp, "'vhost-user' property is required");
> +return;
> +}
> +
> +if (vhost_user_backend_dev_init(vhi->vhost, vdev, 2, errp) == -1) {
> +return;
> +}
> +
> +ret = vhost_user_in
Re: [Qemu-devel] [PATCH for-3.2 07/11] vhost-user: add vhost_user_input_get_config()
On Mon, Nov 26, 2018 at 04:42:46PM +0400, Marc-André Lureau wrote:
> Ask vhost user input backend the list of virtio_input_config.
Reviewed-by: Gerd Hoffmann
>
> Signed-off-by: Marc-André Lureau
> ---
> contrib/libvhost-user/libvhost-user.h | 1 +
> include/hw/virtio/vhost-backend.h | 4 ++
> hw/virtio/vhost-user.c| 60 +++
> docs/interop/vhost-user.txt | 8
> 4 files changed, 73 insertions(+)
>
> diff --git a/contrib/libvhost-user/libvhost-user.h
> b/contrib/libvhost-user/libvhost-user.h
> index 4aa55b4d2d..a4afbc3a46 100644
> --- a/contrib/libvhost-user/libvhost-user.h
> +++ b/contrib/libvhost-user/libvhost-user.h
> @@ -91,6 +91,7 @@ typedef enum VhostUserRequest {
> VHOST_USER_POSTCOPY_ADVISE = 28,
> VHOST_USER_POSTCOPY_LISTEN = 29,
> VHOST_USER_POSTCOPY_END = 30,
> +VHOST_USER_INPUT_GET_CONFIG = 31,
> VHOST_USER_MAX
> } VhostUserRequest;
>
> diff --git a/include/hw/virtio/vhost-backend.h
> b/include/hw/virtio/vhost-backend.h
> index 81283ec50f..1fca321d8a 100644
> --- a/include/hw/virtio/vhost-backend.h
> +++ b/include/hw/virtio/vhost-backend.h
> @@ -12,6 +12,7 @@
> #define VHOST_BACKEND_H
>
> #include "exec/memory.h"
> +#include "standard-headers/linux/virtio_input.h"
>
> typedef enum VhostBackendType {
> VHOST_BACKEND_TYPE_NONE = 0,
> @@ -160,4 +161,7 @@ int vhost_backend_invalidate_device_iotlb(struct
> vhost_dev *dev,
> int vhost_backend_handle_iotlb_msg(struct vhost_dev *dev,
>struct vhost_iotlb_msg *imsg);
>
> +int vhost_user_input_get_config(struct vhost_dev *dev,
> +struct virtio_input_config **config);
> +
> #endif /* VHOST_BACKEND_H */
> diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c
> index 9256364e30..d51d1087f6 100644
> --- a/hw/virtio/vhost-user.c
> +++ b/hw/virtio/vhost-user.c
> @@ -89,6 +89,7 @@ typedef enum VhostUserRequest {
> VHOST_USER_POSTCOPY_ADVISE = 28,
> VHOST_USER_POSTCOPY_LISTEN = 29,
> VHOST_USER_POSTCOPY_END = 30,
> +VHOST_USER_INPUT_GET_CONFIG = 31,
> VHOST_USER_MAX
> } VhostUserRequest;
>
> @@ -338,6 +339,65 @@ static int vhost_user_write(struct vhost_dev *dev,
> VhostUserMsg *msg,
> return 0;
> }
>
> +static void *vhost_user_read_size(struct vhost_dev *dev, uint32_t size)
> +{
> +struct vhost_user *u = dev->opaque;
> +CharBackend *chr = u->user->chr;
> +int r;
> +uint8_t *p = g_malloc(size);
> +
> +r = qemu_chr_fe_read_all(chr, p, size);
> +if (r != size) {
> +error_report("Failed to read msg payload."
> + " Read %d instead of %u.", r, size);
> +g_free(p);
> +return NULL;
> +}
> +
> +return p;
> +}
> +
> +int vhost_user_input_get_config(struct vhost_dev *dev,
> +struct virtio_input_config **config)
> +{
> +void *p = NULL;
> +VhostUserMsg msg = {
> +.hdr.request = VHOST_USER_INPUT_GET_CONFIG,
> +.hdr.flags = VHOST_USER_VERSION,
> +};
> +
> +if (vhost_user_write(dev, &msg, NULL, 0) < 0) {
> +goto err;
> +}
> +
> +if (vhost_user_read_header(dev, &msg) < 0) {
> +goto err;
> +}
> +
> +if (msg.hdr.request != VHOST_USER_INPUT_GET_CONFIG) {
> +error_report("Received unexpected msg type. Expected %d received %d",
> + VHOST_USER_INPUT_GET_CONFIG, msg.hdr.request);
> +goto err;
> +}
> +
> +if (msg.hdr.size % sizeof(struct virtio_input_config)) {
> +error_report("Invalid msg size");
> +goto err;
> +}
> +
> +p = vhost_user_read_size(dev, msg.hdr.size);
> +if (!p) {
> +goto err;
> +}
> +
> +*config = p;
> +return msg.hdr.size / sizeof(struct virtio_input_config);
> +
> +err:
> +g_free(p);
> +return -1;
> +}
> +
> static int vhost_user_set_log_base(struct vhost_dev *dev, uint64_t base,
> struct vhost_log *log)
> {
> diff --git a/docs/interop/vhost-user.txt b/docs/interop/vhost-user.txt
> index cefec9ffe1..95720be81b 100644
> --- a/docs/interop/vhost-user.txt
> +++ b/docs/interop/vhost-user.txt
> @@ -766,6 +766,14 @@ Master message types
>was previously sent.
>The value returned is an error indication; 0 is success.
>
> + * VHOST_USER_INPUT_GET_CONFIG
> + Id: 31
> + Master payload: N/A
> + Slave payload: (struct virtio_input_config)*
> +
> + Ask vhost user input backend the list of virtio_input_config, in
> + host endianness.
> +
> Slave message types
> ---
>
> --
> 2.20.0.rc1
>
Re: [Qemu-devel] [PATCH] hw/s390/ccw.c: Don't take address of packed members
On Mon, 10 Dec 2018 at 14:13, Cornelia Huck wrote:
>
> On Mon, 10 Dec 2018 13:58:03 +
> Peter Maydell wrote:
>
> > Taking the address of a field in a packed struct is a bad idea, because
> > it might not be actually aligned enough for that pointer type (and
> > thus cause a crash on dereference on some host architectures). Newer
> > versions of clang warn about this.
> >
> > Avoid the problem by using local copies of the PMCW and SCSW
> > struct fields in copy_schib_from_guest() and copy_schib_to_guest().
> >
> > Signed-off-by: Peter Maydell
> > ---
> > This seemed like a not totally ugly and reasonably localised fix
> > that satisfies clang. Oddly, this makes the generated object file
> > 15K smaller (421K vs 406K), so it might even be better code...
>
> Nice :)
>
> >
> > hw/s390x/css.c | 20
> > 1 file changed, 16 insertions(+), 4 deletions(-)
> >
> > diff --git a/hw/s390x/css.c b/hw/s390x/css.c
> > index 04ec5cc9705..ef07691e36b 100644
> > --- a/hw/s390x/css.c
> > +++ b/hw/s390x/css.c
> > @@ -1290,9 +1290,15 @@ void copy_scsw_to_guest(SCSW *dest, const SCSW *src)
> > static void copy_schib_to_guest(SCHIB *dest, const SCHIB *src)
> > {
> > int i;
> > +PMCW srcpmcw, destpmcw;
> > +SCSW srcscsw, destscsw;
>
>
> I would find src_pmcw etc. easier to read. Other opinions?
>
CODING_STYLE's "Naming" section agrees with you...
thanks
-- PMM
Re: [Qemu-devel] [PATCH v3 1/3] target/arm: Introduce arm_hcr_el2_eff
On Thu, 6 Dec 2018 at 17:55, Richard Henderson
wrote:
>
> Replace arm_hcr_el2_{fmo,imo,amo} with a more general routine
> that also takes SCR_EL3.NS (aka arm_is_secure_below_el3) into
> account, as documented for the plethora of bits in HCR_EL2.
>
> Signed-off-by: Richard Henderson
>
> v3: Fix set of bits affected by just TGE.
> Reorder the bits to ascending order.
> Zap VF,VI,VSE when !TGE and ![FIA]MO.
> +/*
> + * Return the effective value of HCR_EL2.
> + * Bits that are not included here:
> + * RW (read from SCR_EL3.RW as needed)
> + */
> +uint64_t arm_hcr_el2_eff(CPUARMState *env)
> +{
> +uint64_t ret = env->cp15.hcr_el2;
> +
> +if (arm_is_secure_below_el3(env)) {
> +/*
> + * "This register has no effect if EL2 is not enabled in the
> + * current Security state". This is ARMv8.4-SecEL2 speak for
> + * !(SCR_EL3.NS==1 || SCR_EL3.EEL2==1).
> + *
> + * Prior to that, the language was "In an implementation that
> + * includes EL3, when the value of SCR_EL3.NS is 0 the PE behaves
> + * as if this field is 0 for all purposes other than a direct
> + * read or write access of HCR_EL2". With lots of enumeration
> + * on a per-field basis. In current QEMU, this is condition
> + * is arm_is_secure_below_el3.
> + *
> + * Since the v8.4 language applies to the entire register, and
> + * appears to be backward compatible, use that.
> + */
> +ret = 0;
> +} else if (ret & HCR_TGE) {
> +/* These bits are up-to-date as of ARMv8.4. */
> +if (ret & HCR_E2H) {
> +ret &= ~(HCR_VM | HCR_FMO | HCR_IMO | HCR_AMO |
> + HCR_BSU_MASK | HCR_DC | HCR_TWI | HCR_TWE |
> + HCR_TID0 | HCR_TID2 | HCR_TPCP | HCR_TPU |
> + HCR_TDZ | HCR_CD | HCR_ID | HCR_MIOCNCE);
> +} else {
> +ret |= HCR_FMO | HCR_IMO | HCR_AMO;
> +}
> +ret &= ~(HCR_SWIO | HCR_PTW | HCR_VF | HCR_VI | HCR_VSE |
> + HCR_FB | HCR_TID1 | HCR_TID3 | HCR_TSC | HCR_TACR |
> + HCR_TSW | HCR_TTLB | HCR_TVM | HCR_HCD | HCR_TRVM |
> + HCR_TLOR);
> +} else {
> +if (!(ret & HCR_FMO)) {
> +ret &= ~HCR_VF;
> +}
> +if (!(ret & HCR_IMO)) {
> +ret &= ~HCR_VI;
> +}
> +if (!(ret & HCR_AMO)) {
> +ret &= ~HCR_VSE;
> +}
This section that clears VI/VF/VSE is new, and I'm not sure it's right.
The spec says that the virtual IRQ interrupt is enabled only if {TGE,IMO}
is {0,1}, but the meaning of the bit is "pending", and an interrupt
can be pending without being enabled. Ditto VF, VSE.
(As there are only two places that look at the VI/VF/VSE bits and
they both look directly at cp15.hcr_el2 this doesn't change behaviour.)
> +}
> +
> +return ret;
> +}
> +
thanks
-- PMM
Re: [Qemu-devel] [PATCH] hw/s390/ccw.c: Don't take address of packed members
On Mon, 10 Dec 2018 13:58:03 +
Peter Maydell wrote:
> Taking the address of a field in a packed struct is a bad idea, because
> it might not be actually aligned enough for that pointer type (and
> thus cause a crash on dereference on some host architectures). Newer
> versions of clang warn about this.
>
> Avoid the problem by using local copies of the PMCW and SCSW
> struct fields in copy_schib_from_guest() and copy_schib_to_guest().
>
> Signed-off-by: Peter Maydell
> ---
> This seemed like a not totally ugly and reasonably localised fix
> that satisfies clang. Oddly, this makes the generated object file
> 15K smaller (421K vs 406K), so it might even be better code...
Nice :)
>
> hw/s390x/css.c | 20
> 1 file changed, 16 insertions(+), 4 deletions(-)
>
> diff --git a/hw/s390x/css.c b/hw/s390x/css.c
> index 04ec5cc9705..ef07691e36b 100644
> --- a/hw/s390x/css.c
> +++ b/hw/s390x/css.c
> @@ -1290,9 +1290,15 @@ void copy_scsw_to_guest(SCSW *dest, const SCSW *src)
> static void copy_schib_to_guest(SCHIB *dest, const SCHIB *src)
> {
> int i;
> +PMCW srcpmcw, destpmcw;
> +SCSW srcscsw, destscsw;
I would find src_pmcw etc. easier to read. Other opinions?
>
> -copy_pmcw_to_guest(&dest->pmcw, &src->pmcw);
> -copy_scsw_to_guest(&dest->scsw, &src->scsw);
> +srcpmcw = src->pmcw;
> +copy_pmcw_to_guest(&destpmcw, &srcpmcw);
> +dest->pmcw = destpmcw;
> +srcscsw = src->scsw;
> +copy_scsw_to_guest(&destscsw, &srcscsw);
> +dest->scsw = destscsw;
> dest->mba = cpu_to_be64(src->mba);
> for (i = 0; i < ARRAY_SIZE(dest->mda); i++) {
> dest->mda[i] = src->mda[i];
> @@ -1339,9 +1345,15 @@ static void copy_scsw_from_guest(SCSW *dest, const
> SCSW *src)
> static void copy_schib_from_guest(SCHIB *dest, const SCHIB *src)
> {
> int i;
> +PMCW srcpmcw, destpmcw;
> +SCSW srcscsw, destscsw;
>
> -copy_pmcw_from_guest(&dest->pmcw, &src->pmcw);
> -copy_scsw_from_guest(&dest->scsw, &src->scsw);
> +srcpmcw = src->pmcw;
> +copy_pmcw_from_guest(&destpmcw, &srcpmcw);
> +dest->pmcw = destpmcw;
> +srcscsw = src->scsw;
> +copy_scsw_from_guest(&destscsw, &srcscsw);
> +dest->scsw = destscsw;
> dest->mba = be64_to_cpu(src->mba);
> for (i = 0; i < ARRAY_SIZE(dest->mda); i++) {
> dest->mda[i] = src->mda[i];
[Qemu-devel] [PATCH] input: avoid malloc for mouse events
There is no reason to allocate mouse events using malloc, we can
allcoate them from stack instead, save a few cpu cycles and make the
code more readable with c99 initializers.
Suggested-by: FelixYao
Signed-off-by: Gerd Hoffmann
---
include/ui/input.h | 3 ---
ui/input.c | 68 +-
2 files changed, 31 insertions(+), 40 deletions(-)
diff --git a/include/ui/input.h b/include/ui/input.h
index 34ebc67c5a..8c8ccb999f 100644
--- a/include/ui/input.h
+++ b/include/ui/input.h
@@ -49,7 +49,6 @@ int qemu_input_key_value_to_scancode(const KeyValue *value,
bool down,
int *codes);
int qemu_input_linux_to_qcode(unsigned int lnx);
-InputEvent *qemu_input_event_new_btn(InputButton btn, bool down);
void qemu_input_queue_btn(QemuConsole *src, InputButton btn, bool down);
void qemu_input_update_buttons(QemuConsole *src, uint32_t *button_map,
uint32_t button_old, uint32_t button_new);
@@ -58,8 +57,6 @@ bool qemu_input_is_absolute(void);
int qemu_input_scale_axis(int value,
int min_in, int max_in,
int min_out, int max_out);
-InputEvent *qemu_input_event_new_move(InputEventKind kind,
- InputAxis axis, int value);
void qemu_input_queue_rel(QemuConsole *src, InputAxis axis, int value);
void qemu_input_queue_abs(QemuConsole *src, InputAxis axis, int value,
int min_in, int max_in);
diff --git a/ui/input.c b/ui/input.c
index 7c9a4109c4..f9d413fef9 100644
--- a/ui/input.c
+++ b/ui/input.c
@@ -458,22 +458,18 @@ void qemu_input_event_send_key_delay(uint32_t delay_ms)
}
}
-InputEvent *qemu_input_event_new_btn(InputButton btn, bool down)
-{
-InputEvent *evt = g_new0(InputEvent, 1);
-evt->u.btn.data = g_new0(InputBtnEvent, 1);
-evt->type = INPUT_EVENT_KIND_BTN;
-evt->u.btn.data->button = btn;
-evt->u.btn.data->down = down;
-return evt;
-}
-
void qemu_input_queue_btn(QemuConsole *src, InputButton btn, bool down)
{
-InputEvent *evt;
-evt = qemu_input_event_new_btn(btn, down);
-qemu_input_event_send(src, evt);
-qapi_free_InputEvent(evt);
+InputBtnEvent bevt = {
+.button = btn,
+.down = down,
+};
+InputEvent evt = {
+.type = INPUT_EVENT_KIND_BTN,
+.u.btn.data = &bevt,
+};
+
+qemu_input_event_send(src, &evt);
}
void qemu_input_update_buttons(QemuConsole *src, uint32_t *button_map,
@@ -513,37 +509,35 @@ int qemu_input_scale_axis(int value,
return ((int64_t)value - min_in) * range_out / range_in + min_out;
}
-InputEvent *qemu_input_event_new_move(InputEventKind kind,
- InputAxis axis, int value)
-{
-InputEvent *evt = g_new0(InputEvent, 1);
-InputMoveEvent *move = g_new0(InputMoveEvent, 1);
-
-evt->type = kind;
-evt->u.rel.data = move; /* evt->u.rel is the same as evt->u.abs */
-move->axis = axis;
-move->value = value;
-return evt;
-}
-
void qemu_input_queue_rel(QemuConsole *src, InputAxis axis, int value)
{
-InputEvent *evt;
-evt = qemu_input_event_new_move(INPUT_EVENT_KIND_REL, axis, value);
-qemu_input_event_send(src, evt);
-qapi_free_InputEvent(evt);
+InputMoveEvent move = {
+.axis = axis,
+.value = value,
+};
+InputEvent evt = {
+.type = INPUT_EVENT_KIND_REL,
+.u.rel.data = &move,
+};
+
+qemu_input_event_send(src, &evt);
}
void qemu_input_queue_abs(QemuConsole *src, InputAxis axis, int value,
int min_in, int max_in)
{
-InputEvent *evt;
-int scaled = qemu_input_scale_axis(value, min_in, max_in,
+InputMoveEvent move = {
+.axis = axis,
+.value = qemu_input_scale_axis(value, min_in, max_in,
INPUT_EVENT_ABS_MIN,
- INPUT_EVENT_ABS_MAX);
-evt = qemu_input_event_new_move(INPUT_EVENT_KIND_ABS, axis, scaled);
-qemu_input_event_send(src, evt);
-qapi_free_InputEvent(evt);
+ INPUT_EVENT_ABS_MAX),
+};
+InputEvent evt = {
+.type = INPUT_EVENT_KIND_ABS,
+.u.abs.data = &move,
+};
+
+qemu_input_event_send(src, &evt);
}
void qemu_input_check_mode_change(void)
--
2.9.3
Re: [Qemu-devel] [PATCH v3 5/5] crypto: support multiple threads accessing one QCryptoBlock
On Fri 07 Dec 2018 05:13:51 PM CET, Vladimir Sementsov-Ogievskiy wrote:
> @@ -148,12 +154,97 @@ int qcrypto_block_encrypt(QCryptoBlock *block,
>
> QCryptoCipher *qcrypto_block_get_cipher(QCryptoBlock *block)
> {
> -return block->cipher;
> +/* Ciphers should be accessed through pop/push method to be thread-safe.
> + * Better, they should not be accessed externally at all (note, that
> + * pop/push are static functions)
> + * This function is used only in test with one thread (it's safe to skip
> + * pop/push interface), so it's enough to assert it here:
> + */
> +assert(block->n_ciphers <= 1);
> +return block->ciphers ? block->ciphers[0] : NULL;
If this is only supposed to be called in test mode I think you can also
assert that g_test_initialized() is true.
And the same with qcrypto_block_get_ivgen() later in this patch.
> +int qcrypto_block_init_cipher(QCryptoBlock *block,
> + QCryptoCipherAlgorithm alg,
> + QCryptoCipherMode mode,
> + const uint8_t *key, size_t nkey,
> + size_t n_threads, Error **errp)
> +{
> +size_t i;
> +
> +assert(!block->ciphers && !block->n_ciphers && !block->n_free_ciphers);
> +
> +block->ciphers = g_new0(QCryptoCipher *, n_threads);
You can use g_new() instead of g_new0() because you're anyway
overwriting all elements of the array.
But these are minor nits, the patchs looks good to me else.
Reviewed-by: Alberto Garcia
Berto
[Qemu-devel] [PATCH] hw/s390/ccw.c: Don't take address of packed members
Taking the address of a field in a packed struct is a bad idea, because
it might not be actually aligned enough for that pointer type (and
thus cause a crash on dereference on some host architectures). Newer
versions of clang warn about this.
Avoid the problem by using local copies of the PMCW and SCSW
struct fields in copy_schib_from_guest() and copy_schib_to_guest().
Signed-off-by: Peter Maydell
---
This seemed like a not totally ugly and reasonably localised fix
that satisfies clang. Oddly, this makes the generated object file
15K smaller (421K vs 406K), so it might even be better code...
hw/s390x/css.c | 20
1 file changed, 16 insertions(+), 4 deletions(-)
diff --git a/hw/s390x/css.c b/hw/s390x/css.c
index 04ec5cc9705..ef07691e36b 100644
--- a/hw/s390x/css.c
+++ b/hw/s390x/css.c
@@ -1290,9 +1290,15 @@ void copy_scsw_to_guest(SCSW *dest, const SCSW *src)
static void copy_schib_to_guest(SCHIB *dest, const SCHIB *src)
{
int i;
+PMCW srcpmcw, destpmcw;
+SCSW srcscsw, destscsw;
-copy_pmcw_to_guest(&dest->pmcw, &src->pmcw);
-copy_scsw_to_guest(&dest->scsw, &src->scsw);
+srcpmcw = src->pmcw;
+copy_pmcw_to_guest(&destpmcw, &srcpmcw);
+dest->pmcw = destpmcw;
+srcscsw = src->scsw;
+copy_scsw_to_guest(&destscsw, &srcscsw);
+dest->scsw = destscsw;
dest->mba = cpu_to_be64(src->mba);
for (i = 0; i < ARRAY_SIZE(dest->mda); i++) {
dest->mda[i] = src->mda[i];
@@ -1339,9 +1345,15 @@ static void copy_scsw_from_guest(SCSW *dest, const SCSW
*src)
static void copy_schib_from_guest(SCHIB *dest, const SCHIB *src)
{
int i;
+PMCW srcpmcw, destpmcw;
+SCSW srcscsw, destscsw;
-copy_pmcw_from_guest(&dest->pmcw, &src->pmcw);
-copy_scsw_from_guest(&dest->scsw, &src->scsw);
+srcpmcw = src->pmcw;
+copy_pmcw_from_guest(&destpmcw, &srcpmcw);
+dest->pmcw = destpmcw;
+srcscsw = src->scsw;
+copy_scsw_from_guest(&destscsw, &srcscsw);
+dest->scsw = destscsw;
dest->mba = be64_to_cpu(src->mba);
for (i = 0; i < ARRAY_SIZE(dest->mda); i++) {
dest->mda[i] = src->mda[i];
--
2.19.2
Re: [Qemu-devel] [PATCH v3 3/3] hw/s390x/ioinst: Fix alignment problem in struct SubchDev
On Mon, 10 Dec 2018 at 13:32, Dr. David Alan Gilbert
wrote:
> Is the problem here that the field could actually be misaligned (on
> any conceivable build) or is it just a matter of convincing clang it's
> safe?
This is mostly a "clang doesn't know that the struct field
will actually always be 4 aligned and the function being
called only requires 4 alignment" thing. But there is an
actual-in-theory problem too: because the SCHIB struct is
marked QEMU_PACKED it has no alignment requirements at all.
So in for instance css_do_msch() the compiler is entitled
to align the local "SCHIB schib" at any alignment it likes,
which may not be the 4-alignment that copy_pmcw_from_guest()
assumes.
An option (3) which I hadn't previously thought of:
copy the pmcw and scsw fields into and out of locals
which are guaranteed to be correctly aligned:
--- a/hw/s390x/css.c
+++ b/hw/s390x/css.c
@@ -1290,9 +1290,15 @@ void copy_scsw_to_guest(SCSW *dest, const SCSW *src)
static void copy_schib_to_guest(SCHIB *dest, const SCHIB *src)
{
int i;
+PMCW srcpmcw, destpmcw;
+SCSW srcscsw, destscsw;
-copy_pmcw_to_guest(&dest->pmcw, &src->pmcw);
-copy_scsw_to_guest(&dest->scsw, &src->scsw);
+srcpmcw = src->pmcw;
+copy_pmcw_to_guest(&destpmcw, &srcpmcw);
+dest->pmcw = destpmcw;
+srcscsw = src->scsw;
+copy_scsw_to_guest(&destscsw, &srcscsw);
+dest->scsw = destscsw;
dest->mba = cpu_to_be64(src->mba);
for (i = 0; i < ARRAY_SIZE(dest->mda); i++) {
dest->mda[i] = src->mda[i];
@@ -1339,9 +1345,15 @@ static void copy_scsw_from_guest(SCSW *dest,
const SCSW *src)
static void copy_schib_from_guest(SCHIB *dest, const SCHIB *src)
{
int i;
+PMCW srcpmcw, destpmcw;
+SCSW srcscsw, destscsw;
-copy_pmcw_from_guest(&dest->pmcw, &src->pmcw);
-copy_scsw_from_guest(&dest->scsw, &src->scsw);
+srcpmcw = src->pmcw;
+copy_pmcw_from_guest(&destpmcw, &srcpmcw);
+dest->pmcw = destpmcw;
+srcscsw = src->scsw;
+copy_scsw_from_guest(&destscsw, &srcscsw);
+dest->scsw = destscsw;
dest->mba = be64_to_cpu(src->mba);
for (i = 0; i < ARRAY_SIZE(dest->mda); i++) {
dest->mda[i] = src->mda[i];
thanks
-- PMM
Re: [Qemu-devel] QEMU/NEMU boot time with several x86 firmwares
Hi Maran, On Wed, Dec 5, 2018 at 7:04 PM Maran Wilson wrote: > > On 12/5/2018 5:20 AM, Stefan Hajnoczi wrote: > > On Tue, Dec 04, 2018 at 02:44:33PM -0800, Maran Wilson wrote: > >> On 12/3/2018 8:35 AM, Stefano Garzarella wrote: > >>> On Mon, Dec 3, 2018 at 4:44 PM Rob Bradford > >>> wrote: > Hi Stefano, thanks for capturing all these numbers, > > On Mon, 2018-12-03 at 15:27 +0100, Stefano Garzarella wrote: > > Hi Rob, > > I continued to investigate the boot time, and as you suggested I > > looked also at qemu-lite 2.11.2 > > (https://github.com/kata-containers/qemu) and NEMU "virt" machine. I > > did the following tests using the Kata kernel configuration > > ( > > https://github.com/kata-containers/packaging/blob/master/kernel/configs/x86_64_kata_kvm_4.14.x > > ) > > > > To compare the results with qemu-lite direct kernel load, I added > > another tracepoint: > > - linux_start_kernel: first entry of the Linux kernel > > (start_kernel()) > > > Great, do you have a set of patches available that all these trace > points. It would be great for reproduction. > >>> For sure! I'm attaching a set of patches for qboot, seabios, ovmf, > >>> nemu/qemu/qemu-lite and linux 4.14 whit the tracepoints. > >>> I'm also sharing a python script that I'm using with perf to extract > >>> the numbers in this way: > >>> > >>> $ perf record -a -e kvm:kvm_entry -e kvm:kvm_pio -e > >>> sched:sched_process_exec -o /tmp/qemu_perf.data & > >>> $ # start qemu/nemu multiple times > >>> $ killall perf > >>> $ perf script -s qemu-perf-script.py -i /tmp/qemu_perf.data > >>> > > As you can see, NEMU is faster to jump to the kernel > > (linux_start_kernel) than qemu-lite when uses qboot or seabios with > > virt support, but the time to the user space is strangely high, maybe > > the kernel configuration that I used is not the best one. > > Do you suggest another kernel configuration? > > > This looks very bad. This isn't the kernel configuration we normally > test with in our automated test system but is definitely one we support > as part of our partnernship with the Kata team. It's a high priority > for me to try and investigate that. Have you saved the kernel messages > as they might be helpful? > >>> Yes, I'm attaching the dmesg output with nemu and qemu. > >>> > > Anyway, I obtained the best boot time with qemu-lite and direct > > kernel > > load (vmlinux ELF image). I think because the kernel was not > > compressed. Indeed, looking to the others test, the kernel > > decompression (bzImage) takes about 80 ms (linux_start_kernel - > > linux_start_boot). (I'll investigate better) > > > Yup being able to load an uncompressed kernel is one of the big > advantages of qemu-lite. I wonder if we could bring that feature into > qemu itself to supplement the existing firmware based kernel loading. > >>> I think so, I'll try to understand if we can merge the qemu-lite > >>> direct kernel loading in qemu. > >> An attempt was made a long time ago to push the qemu-lite stuff (from the > >> Intel Clear Containers project) upstream. As I understand it, the main > >> stumbling block that seemed to derail the effort was that it involved > >> adding > >> Linux OS specific code to Qemu so that Qemu could do things like create and > >> populate the zero page that Linux expects when entering startup_64(). > >> > >> That ends up being a lot of very low-level, operating specific knowledge > >> about Linux that ends up getting baked into Qemu code. And understandably, > >> a > >> number of folks saw problems with going down a path like that. > >> > >> Since then, we have put together an alternative solution that would allow > >> Qemu to boot an uncompressed Linux binary via the x86/HVM direct boot ABI > >> (https://xenbits.xen.org/docs/unstable/misc/pvh.html). The solution > >> involves > >> first making changes to both the ABI as well as Linux, and then updating > >> Qemu to take advantage of the updated ABI which is already supported by > >> both > >> Linux and Free BSD for booting VMs. As such, Qemu can remain OS agnostic, > >> and just be programmed to the published ABI. > >> > >> The canonical definition for the HVM direct boot ABI is in the Xen tree and > >> we needed to make some minor changes to the ABI definition to allow KVM > >> guests to also use the same structure and entry point. Those changes were > >> accepted to the Xen tree already: > >> https://lists.xenproject.org/archives/html/xen-devel/2018-04/msg00057.html > >> > >> The corresponding Linux changes that would allow KVM guests to be booted > >> via > >> this PVH entry point have already been posted and reviewed: > >> https://lkml.org/lkml/2018/4/16/1002 > >> > >> The final part is the set of Qemu changes to take advantage of the above > >> and > >> boot a KVM guest via an uncompressed kernel binary using the e
Re: [Qemu-devel] [qemu-s390x] [PATCH] hw/s390x/virtio-ccw.c: Don't take address of fields in packed structs
On Mon, 10 Dec 2018 14:06:42 +0100 Halil Pasic wrote: > On Mon, 10 Dec 2018 12:04:36 + > Peter Maydell wrote: > > > Taking the address of a field in a packed struct is a bad idea, because > > it might not be actually aligned enough for that pointer type (and > > thus cause a crash on dereference on some host architectures). Newer > > versions of clang warn about this. Avoid the bug by not using the > > "modify in place" byte swapping functions. > > > > Patch produced with scripts/coccinelle/inplace-byteswaps.cocci > > (with a couple of long lines manually wrapped). > > > > Signed-off-by: Peter Maydell > > Reviewed-by: Halil Pasic > > I wonder if packed was a good idea in the first place. @Connie: Do you > have an opinion on this? Some of the structures probably don't need the packed attribute; but there are two structures that have a trailing 8 bit value. The problem is that the structures are a a guest<->host interface as defined in the virtio standard, so we can't e.g. add padding to the structures with the isc at the end. If I had to re-define the interface, I'd probably add tail padding; but I was not aware of the problems the packing might cause when I defined them, and we can't get rid of them without also getting rid of support for older virtio versions. Simply moving away from the modify in place byteswaps is the easiest solution.
Re: [Qemu-devel] qdev documentation
On Dec 9, 2018 5:00 PM, "BALATON Zoltan" wrote: > > Hello, > > I was searching for some good summary on how to implement new devices and machines in QEMU for those who might want to start working on it but not much seems to be available. I've found this which seems to be useful: > > https://lists.nongnu.org/archive/html/qemu-devel/2011-07/msg00842.html > > What happened to this? Was it ever finished? It does not seem to be in docs, only qdev-device-use.txt is there. (Maybe this qdev-for-programmers doc could be in docs/devel.) > > Thank you, > BALATON Zoltan > Hi, Zoltan. I unfortunatelly can't answer your questions, but I do agree that some writings on QEMU internals would be super-helpful. The problem is that, as we all know, documentation often gets forgotten in our hectic pace of development, even if it is planned to be written. It could be that a problem is there is no a single person (at least I don't know him/her) who would actively take care of the internal docs as a whole, and, let's say, constantly ping other developers to improve or update their bits and pieces. Good that you brought this to our attention! Aleksandar
Re: [Qemu-devel] [PATCH] usb-host: reset and close libusb_device_handle before qemu exit
On Fri, Nov 30, 2018 at 02:47:00PM +0800, linzhecheng wrote:
> we should perform these things as same as usb_host_close.
>
> Signed-off-by: linzhecheng
>
> diff --git a/hw/usb/host-libusb.c b/hw/usb/host-libusb.c
> index b6602ded4e..2016375e6b 100644
> --- a/hw/usb/host-libusb.c
> +++ b/hw/usb/host-libusb.c
> @@ -988,7 +988,9 @@ static void usb_host_exit_notifier(struct Notifier *n,
> void *data)
>
> if (s->dh) {
> usb_host_release_interfaces(s);
> + libusb_reset_device(s->dh);
> usb_host_attach_kernel(s);
> + libusb_close(s->dh);
qemu codesstyle uses spaces not tabs, please update your editor settings.
Fixed and added to usb queue.
thanks,
Gerd
Re: [Qemu-devel] [PATCH v3 3/5] crypto/block: rename qcrypto_block_*crypt_helper
On Mon 10 Dec 2018 02:21:56 PM CET, Daniel P. Berrangé wrote: > On Mon, Dec 10, 2018 at 11:44:22AM +0100, Alberto Garcia wrote: >> On Fri 07 Dec 2018 05:13:49 PM CET, Vladimir Sementsov-Ogievskiy wrote: >> > Rename qcrypto_block_*crypt_helper to qcrypto_cipher_*crypt_helper, as >> >> You forgot to update the new function names in the commit message. >> >> > -int qcrypto_block_decrypt_helper(QCryptoCipher *cipher, >> > -int qcrypto_block_encrypt_helper(QCryptoCipher *cipher, >> > +int qcrypto_block_cipher_decrypt_helper(QCryptoCipher *cipher, >> > +int qcrypto_block_cipher_encrypt_helper(QCryptoCipher *cipher, > > I'll take care of that when queuing these patches Ok then Reviewed-by: Alberto Garcia Berto
Re: [Qemu-devel] [PATCH for-4.0 v3] configure: bump spice-server required version to 0.12.5
On Wed, Nov 28, 2018 at 07:59:32PM +0400, Marc-André Lureau wrote: > Looking at chardev/spice.c code, I realize compilation was broken for > a while with spice-server < 0.12.3. Let's bump required version > to 0.12.5, released May 19 2014, instead of adding more #ifdef. > > (this patch combines changes from an early version and some of > Frediano "[PATCH 2/2] spice: Bump required spice-server version to > 0.12.6") > > According to repology, all the distros that are build target platforms > for QEMU include it: > > RHEL-7: 0.14.0 > Debian (Stretch): 0.12.8 > Debian (Jessie): 0.12.5 > FreeBSD (ports): 0.14.0 > OpenSUSE Leap 15: 0.14.0 > Ubuntu (Xenial): 0.12.6 > > Note that a previous version of this patch was bumping version to > 0.12.6. Unfortunately, Debian Jessie (oldstable) is stuck with spice > server 0.12.5, and QEMU should keep building until after 2y of current > stable (Stretch), which will be around June 17th 2019. Qemu 4.1 > should thus be free of bumping to spice-server 0.12.6 during 4.1 > development cycle. Added to UI queue. thanks, Gerd
Re: [Qemu-devel] [PATCH for-4.0 v7 01/27] qapi: make sure osdep.h is included in type headers
Marc-André Lureau writes:
> Hi
>
> On Mon, Dec 10, 2018 at 1:52 PM Markus Armbruster wrote:
>>
>> Marc-André Lureau writes:
>>
>> > Now that the schema can be configured, it is crucial that all types
>> > are configured the same. Make sure config-host.h is included, by
>> > checking osdep.h inclusion. The build-sys tracks the dependency and
>> > rebuilds the types if the configuration changed.
>> >
>> > Signed-off-by: Marc-André Lureau
>> > ---
>> > scripts/qapi/types.py | 3 +++
>> > 1 file changed, 3 insertions(+)
>> >
>> > diff --git a/scripts/qapi/types.py b/scripts/qapi/types.py
>> > index fd7808103c..3bb9cb6d47 100644
>> > --- a/scripts/qapi/types.py
>> > +++ b/scripts/qapi/types.py
>> > @@ -201,6 +201,9 @@ class
>> > QAPISchemaGenTypeVisitor(QAPISchemaModularCVisitor):
>> > ''',
>> >types=types, visit=visit))
>> > self._genh.preamble_add(mcgen('''
>> > +#ifndef QEMU_OSDEP_H
>> > +#error Please include osdep.h first!
>> > +#endif
>> > #include "qapi/qapi-builtin-types.h"
>> > '''))
>>
>> I understand why you propose this patch. The QAPI-generated headers use
>> #ifdef CONFIG_FOO. The configuration header "qemu/osdep.h" must be
>> consistently included before the generated headers, or else you end up
>> with nasty bugs, such as the same enum having different values in
>> different .o, or the same struct having a different layout.
>>
>> But this applies to *all* headers that use #ifdef. Why check it here,
>> but not there? What makes the QAPI-generated headers special?
>>
>
> It's the discussion about #if in headers (and enums in particular)
> that started this. We want to make sure all compilation units share
> the same data structure/ABI. I proposed to include osdep.h in qapi
> headers, but that was rejected.
> The warning is a different approach. I agree it could apply to all
> headers. Do you think I should update all headers as well?
That would replace the rule 'all .c files must include "qemu/osdep.h"
first' by 'all .h files must check "qemu/osdep.h" has been included
already', which is not an improvement.
All we have right now to help with enforcing our osdep.h rule is
scripts/clean-includes. We run it periodically to fix rule breakers.
It's manual, because we have a few .c files in the tree where the rule
doesn't apply, and the script doesn't know about them. Fixable, I
guess. Most recent run:
Subject: [PATCH] Clean up includes
Message-Id: <20181204172535.2799-1-arm...@redhat.com>
https://lists.nongnu.org/archive/html/qemu-devel/2018-12/msg00605.html
The obvious improvement would be flagging rule breakers before they get
committed.
checkpatch.pl could flag patches adding .c files that don't include
"qemu/osdep.h" first. The "first" part might be a bit annoying to code.
The "adding files" part already exists: checkpatch.pl warns when you add
a file without updating MAINTAINERS.
checkpatch.pl could also flag patches removing #include "qemu/osdep.h".
Other projects use a syntax-check make target to check sources. If we
decide we like that better than checkpatch.pl for some checks, we can do
that.
Re: [Qemu-devel] [PATCH for-4.0] tests/display-vga: Enable virtio-vga test
On Thu, Nov 29, 2018 at 12:50:48PM +0100, Thomas Huth wrote: > There are some "#ifdef CONFIG_VIRTIO_VGA" in the code here which > do not work as expected: CONFIG_VIRTIO_VGA is a Makefile switch, > but not a CPP macro, so the "guarded" code currently simply never > gets enabled. > > So enable this code now unconditionally, with some runtime switches > for the architectures that have the VIRTIO_VGA device enabled by > default. Looking at the other if-statement in the main function here, > it also seems like this test was originally supposed to be running > on "mips" and "alpha", too, so enable it now for these architectures > in the Makefile, too. Added to vga queue. thanks, Gerd
Re: [Qemu-devel] [PATCH v3 3/3] hw/s390x/ioinst: Fix alignment problem in struct SubchDev
* Cornelia Huck (coh...@redhat.com) wrote:
> On Mon, 10 Dec 2018 12:27:56 +
> Peter Maydell wrote:
>
> > On Thu, 27 Sep 2018 at 09:25, Thomas Huth wrote:
> > >
> > > struct SubchDev embeds several other structures which are marked with
> > > QEMU_PACKED. This causes the compiler to not care for proper alignment
> > > of these structures. When we later pass around pointers to the unaligned
> > > struct members during migration, this causes problems on host
> > > architectures
> > > like Sparc that can not do unaligned memory access.
> > >
> > > Most of the structs in ioinst.h are naturally aligned, so we can fix
> > > most of the problem by removing the QEMU_PACKED statements (and use
> > > QEMU_BUILD_BUG_MSG() statements instead to make sure that there is no
> > > padding). However, for the struct SCHIB, we have to keep the QEMU_PACKED
> > > since the compiler adds some padding here otherwise. Move this struct
> > > to the beginning of struct SubchDev instead to fix the alignment problem
> > > here, too.
> >
> > Unfortunately clang does not like the struct SCHIB being still
> > marked packed:
> >
> > /home/petmay01/linaro/qemu-from-laptop/qemu/hw/s390x/css.c:1294:25:
> > warning: taking address of packed member 'pmcw' of class or structure
> > 'SCHIB' may result in an unaligned pointer value
> > [-Waddress-of-packed-member]
> > copy_pmcw_to_guest(&dest->pmcw, &src->pmcw);
> > ^~
> > /home/petmay01/linaro/qemu-from-laptop/qemu/hw/s390x/css.c:1294:38:
> > warning: taking address of packed member 'pmcw' of class or structure
> > 'SCHIB' may result in an unaligned pointer value
> > [-Waddress-of-packed-member]
> > copy_pmcw_to_guest(&dest->pmcw, &src->pmcw);
> > ^
> > /home/petmay01/linaro/qemu-from-laptop/qemu/hw/s390x/css.c:1295:25:
> > warning: taking address of packed member 'scsw' of class or structure
> > 'SCHIB' may result in an unaligned pointer value
> > [-Waddress-of-packed-member]
> > copy_scsw_to_guest(&dest->scsw, &src->scsw);
> > ^~
> > /home/petmay01/linaro/qemu-from-laptop/qemu/hw/s390x/css.c:1295:38:
> > warning: taking address of packed member 'scsw' of class or structure
> > 'SCHIB' may result in an unaligned pointer value
> > [-Waddress-of-packed-member]
> > copy_scsw_to_guest(&dest->scsw, &src->scsw);
> > ^
> > /home/petmay01/linaro/qemu-from-laptop/qemu/hw/s390x/css.c:1343:27:
> > warning: taking address of packed member 'pmcw' of class or structure
> > 'SCHIB' may result in an unaligned pointer value
> > [-Waddress-of-packed-member]
> > copy_pmcw_from_guest(&dest->pmcw, &src->pmcw);
> > ^~
> > /home/petmay01/linaro/qemu-from-laptop/qemu/hw/s390x/css.c:1343:40:
> > warning: taking address of packed member 'pmcw' of class or structure
> > 'SCHIB' may result in an unaligned pointer value
> > [-Waddress-of-packed-member]
> > copy_pmcw_from_guest(&dest->pmcw, &src->pmcw);
> >^
> > /home/petmay01/linaro/qemu-from-laptop/qemu/hw/s390x/css.c:1344:27:
> > warning: taking address of packed member 'scsw' of class or structure
> > 'SCHIB' may result in an unaligned pointer value
> > [-Waddress-of-packed-member]
> > copy_scsw_from_guest(&dest->scsw, &src->scsw);
> > ^~
> > /home/petmay01/linaro/qemu-from-laptop/qemu/hw/s390x/css.c:1344:40:
> > warning: taking address of packed member 'scsw' of class or structure
> > 'SCHIB' may result in an unaligned pointer value
> > [-Waddress-of-packed-member]
> > copy_scsw_from_guest(&dest->scsw, &src->scsw);
> >^
>
> That's really annoying :(
Is the problem here that the field could actually be misaligned (on
any conceivable build) or is it just a matter of convincing clang it's
safe?
Dave
> > Not sure how best to address this. A couple of ideas that I had:
> >
> > (1) make the 'uint64_t mba' field in the SCHIB struct into
> > two uint32_t fields, adjusting all the code which needs
> > to access it accordingly; then we could drop the packed
> > annotation from the struct
>
> This would mean some annoying gymnastics, but fortunately that field is
> not accessed in many places.
>
> >
> > (2) have the guts of copy_{pmcw,scsw}_{to,from}_guest() be
> > macros, so we can do them inline in the copy_schib_{to,from}_guest()
> > function and thus operate directly on src->pmcw.foo &c
> > fields rather than ever having to take the address of any
> > of the fields in src or dest
>
> I'm not really a fan of using macros, but if it stays readable...
>
> Not sure what the best option is here; this is why I haven't done
> anything yet to fix it, as no idea was really appealing.
--
Dr. David Alan Gilbert / dgilb...@redhat.com / Manchester, UK
Re: [Qemu-devel] [PATCH v3 3/5] crypto/block: rename qcrypto_block_*crypt_helper
On Mon, Dec 10, 2018 at 11:44:22AM +0100, Alberto Garcia wrote: > On Fri 07 Dec 2018 05:13:49 PM CET, Vladimir Sementsov-Ogievskiy wrote: > > Rename qcrypto_block_*crypt_helper to qcrypto_cipher_*crypt_helper, as > > You forgot to update the new function names in the commit message. > > > -int qcrypto_block_decrypt_helper(QCryptoCipher *cipher, > > -int qcrypto_block_encrypt_helper(QCryptoCipher *cipher, > > +int qcrypto_block_cipher_decrypt_helper(QCryptoCipher *cipher, > > +int qcrypto_block_cipher_encrypt_helper(QCryptoCipher *cipher, I'll take care of that when queuing these patches 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 v3 19/21] usb/tusb6010: Convert sysbus init function to realize function
On Fri, Nov 30, 2018 at 05:38:50PM +0800, Mao Zhongyi wrote:
> Use DeviceClass rather than SysBusDeviceClass in
> tusb6010_class_init().
>
> Cc: kra...@redhat.com
>
> Signed-off-by: Mao Zhongyi
> Signed-off-by: Zhang Shengju
Reviewed-by: Gerd Hoffmann
> ---
> hw/usb/tusb6010.c | 8 +++-
> 1 file changed, 3 insertions(+), 5 deletions(-)
>
> diff --git a/hw/usb/tusb6010.c b/hw/usb/tusb6010.c
> index a2128024c1..501706e2b2 100644
> --- a/hw/usb/tusb6010.c
> +++ b/hw/usb/tusb6010.c
> @@ -808,10 +808,10 @@ static void tusb6010_reset(DeviceState *dev)
> musb_reset(s->musb);
> }
>
> -static int tusb6010_init(SysBusDevice *sbd)
> +static void tusb6010_realize(DeviceState *dev, Error **errp)
> {
> -DeviceState *dev = DEVICE(sbd);
> TUSBState *s = TUSB(dev);
> +SysBusDevice *sbd = SYS_BUS_DEVICE(dev);
>
> s->otg_timer = timer_new_ns(QEMU_CLOCK_VIRTUAL, tusb_otg_tick, s);
> s->pwr_timer = timer_new_ns(QEMU_CLOCK_VIRTUAL, tusb_power_tick, s);
> @@ -822,15 +822,13 @@ static int tusb6010_init(SysBusDevice *sbd)
> sysbus_init_irq(sbd, &s->irq);
> qdev_init_gpio_in(dev, tusb6010_irq, musb_irq_max + 1);
> s->musb = musb_init(dev, 1);
> -return 0;
> }
>
> static void tusb6010_class_init(ObjectClass *klass, void *data)
> {
> DeviceClass *dc = DEVICE_CLASS(klass);
> -SysBusDeviceClass *k = SYS_BUS_DEVICE_CLASS(klass);
>
> -k->init = tusb6010_init;
> +dc->realize = tusb6010_realize;
> dc->reset = tusb6010_reset;
> }
>
> --
> 2.17.1
>
>
>
Re: [Qemu-devel] [PATCH v3 05/21] display/g364fb: Convert sysbus init function to realize function
On Fri, Nov 30, 2018 at 05:38:36PM +0800, Mao Zhongyi wrote:
> Use DeviceClass rather than SysBusDeviceClass in
> g364fb_sysbus_class_init().
>
> Cc: pbonz...@redhat.com
> Cc: kra...@redhat.com
> Cc: f4...@amsat.org
> Cc: alistair.fran...@wdc.com
>
> Signed-off-by: Mao Zhongyi
> Signed-off-by: Zhang Shengju
> Reviewed-by: Alistair Francis
> Reviewed-by: Philippe Mathieu-Daudé
Reviewed-by: Gerd Hoffmann
> ---
> hw/display/g364fb.c | 9 +++--
> 1 file changed, 3 insertions(+), 6 deletions(-)
>
> diff --git a/hw/display/g364fb.c b/hw/display/g364fb.c
> index 8ad7e5d824..3407adf98d 100644
> --- a/hw/display/g364fb.c
> +++ b/hw/display/g364fb.c
> @@ -489,18 +489,16 @@ typedef struct {
> G364State g364;
> } G364SysBusState;
>
> -static int g364fb_sysbus_init(SysBusDevice *sbd)
> +static void g364fb_sysbus_realize(DeviceState *dev, Error **errp)
> {
> -DeviceState *dev = DEVICE(sbd);
> G364SysBusState *sbs = G364(dev);
> G364State *s = &sbs->g364;
> +SysBusDevice *sbd = SYS_BUS_DEVICE(dev);
>
> g364fb_init(dev, s);
> sysbus_init_irq(sbd, &s->irq);
> sysbus_init_mmio(sbd, &s->mem_ctrl);
> sysbus_init_mmio(sbd, &s->mem_vram);
> -
> -return 0;
> }
>
> static void g364fb_sysbus_reset(DeviceState *d)
> @@ -518,9 +516,8 @@ static Property g364fb_sysbus_properties[] = {
> static void g364fb_sysbus_class_init(ObjectClass *klass, void *data)
> {
> DeviceClass *dc = DEVICE_CLASS(klass);
> -SysBusDeviceClass *k = SYS_BUS_DEVICE_CLASS(klass);
>
> -k->init = g364fb_sysbus_init;
> +dc->realize = g364fb_sysbus_realize;
> set_bit(DEVICE_CATEGORY_DISPLAY, dc->categories);
> dc->desc = "G364 framebuffer";
> dc->reset = g364fb_sysbus_reset;
> --
> 2.17.1
>
>
>
Re: [Qemu-devel] [PATCH for-4.0] usb: move ehci_create_ich9_with_companions to hw/i386
On Fri, Nov 30, 2018 at 10:45:12PM +0100, Paolo Bonzini wrote:
> This function is only needed when Q35 is in use. Moving it to
> the same file that uses it lets you disable the entire USB
> subsystem in x86_64-softmmu.mak; of course doing that will
> cause -usb to break horribly, but one thing at a time.
Patch doesn't apply.
> -if (0 && machine_usb(machine)) {
> +if (machine_usb(machine)) {
Leftover local debug change here?
cheers,
Gerd
Re: [Qemu-devel] [PATCH v3 3/3] hw/s390x/ioinst: Fix alignment problem in struct SubchDev
On Mon, 10 Dec 2018 12:27:56 +
Peter Maydell wrote:
> On Thu, 27 Sep 2018 at 09:25, Thomas Huth wrote:
> >
> > struct SubchDev embeds several other structures which are marked with
> > QEMU_PACKED. This causes the compiler to not care for proper alignment
> > of these structures. When we later pass around pointers to the unaligned
> > struct members during migration, this causes problems on host architectures
> > like Sparc that can not do unaligned memory access.
> >
> > Most of the structs in ioinst.h are naturally aligned, so we can fix
> > most of the problem by removing the QEMU_PACKED statements (and use
> > QEMU_BUILD_BUG_MSG() statements instead to make sure that there is no
> > padding). However, for the struct SCHIB, we have to keep the QEMU_PACKED
> > since the compiler adds some padding here otherwise. Move this struct
> > to the beginning of struct SubchDev instead to fix the alignment problem
> > here, too.
>
> Unfortunately clang does not like the struct SCHIB being still
> marked packed:
>
> /home/petmay01/linaro/qemu-from-laptop/qemu/hw/s390x/css.c:1294:25:
> warning: taking address of packed member 'pmcw' of class or structure
> 'SCHIB' may result in an unaligned pointer value
> [-Waddress-of-packed-member]
> copy_pmcw_to_guest(&dest->pmcw, &src->pmcw);
> ^~
> /home/petmay01/linaro/qemu-from-laptop/qemu/hw/s390x/css.c:1294:38:
> warning: taking address of packed member 'pmcw' of class or structure
> 'SCHIB' may result in an unaligned pointer value
> [-Waddress-of-packed-member]
> copy_pmcw_to_guest(&dest->pmcw, &src->pmcw);
> ^
> /home/petmay01/linaro/qemu-from-laptop/qemu/hw/s390x/css.c:1295:25:
> warning: taking address of packed member 'scsw' of class or structure
> 'SCHIB' may result in an unaligned pointer value
> [-Waddress-of-packed-member]
> copy_scsw_to_guest(&dest->scsw, &src->scsw);
> ^~
> /home/petmay01/linaro/qemu-from-laptop/qemu/hw/s390x/css.c:1295:38:
> warning: taking address of packed member 'scsw' of class or structure
> 'SCHIB' may result in an unaligned pointer value
> [-Waddress-of-packed-member]
> copy_scsw_to_guest(&dest->scsw, &src->scsw);
> ^
> /home/petmay01/linaro/qemu-from-laptop/qemu/hw/s390x/css.c:1343:27:
> warning: taking address of packed member 'pmcw' of class or structure
> 'SCHIB' may result in an unaligned pointer value
> [-Waddress-of-packed-member]
> copy_pmcw_from_guest(&dest->pmcw, &src->pmcw);
> ^~
> /home/petmay01/linaro/qemu-from-laptop/qemu/hw/s390x/css.c:1343:40:
> warning: taking address of packed member 'pmcw' of class or structure
> 'SCHIB' may result in an unaligned pointer value
> [-Waddress-of-packed-member]
> copy_pmcw_from_guest(&dest->pmcw, &src->pmcw);
>^
> /home/petmay01/linaro/qemu-from-laptop/qemu/hw/s390x/css.c:1344:27:
> warning: taking address of packed member 'scsw' of class or structure
> 'SCHIB' may result in an unaligned pointer value
> [-Waddress-of-packed-member]
> copy_scsw_from_guest(&dest->scsw, &src->scsw);
> ^~
> /home/petmay01/linaro/qemu-from-laptop/qemu/hw/s390x/css.c:1344:40:
> warning: taking address of packed member 'scsw' of class or structure
> 'SCHIB' may result in an unaligned pointer value
> [-Waddress-of-packed-member]
> copy_scsw_from_guest(&dest->scsw, &src->scsw);
>^
That's really annoying :(
> Not sure how best to address this. A couple of ideas that I had:
>
> (1) make the 'uint64_t mba' field in the SCHIB struct into
> two uint32_t fields, adjusting all the code which needs
> to access it accordingly; then we could drop the packed
> annotation from the struct
This would mean some annoying gymnastics, but fortunately that field is
not accessed in many places.
>
> (2) have the guts of copy_{pmcw,scsw}_{to,from}_guest() be
> macros, so we can do them inline in the copy_schib_{to,from}_guest()
> function and thus operate directly on src->pmcw.foo &c
> fields rather than ever having to take the address of any
> of the fields in src or dest
I'm not really a fan of using macros, but if it stays readable...
Not sure what the best option is here; this is why I haven't done
anything yet to fix it, as no idea was really appealing.
Re: [Qemu-devel] [PATCH] pvusb: set max grants only in initialise
On Thu, Dec 06, 2018 at 02:39:23PM +0100, Juergen Gross wrote: > Don't call xen_be_set_max_grant_refs() in usbback_alloc(), as the > gnttabdev pointer won't be initialised yet. The call can easily be > moved to usbback_connect(). Added to usb queue. thanks, Gerd
Re: [Qemu-devel] [PATCH v4] qemu-img info lists bitmap directory entries
Please don't use "Content-Transfer-Encoding: base64".
Andrey Shinkevich writes:
> On 07.12.2018 19:20, Eric Blake wrote:
>
>> On 12/7/18 4:00 AM, Andrey Shinkevich wrote:
[...]
>>> +++ b/block/qcow2.c
>>> @@ -4270,6 +4270,12 @@ static ImageInfoSpecific
>>> *qcow2_get_specific_info(BlockDriverState *bs)
>>> .refcount_bits = s->refcount_bits,
>>> };
>>> } else if (s->qcow_version == 3) {
>>> + Qcow2BitmapInfoList *bitmaps;
>>> + Error *local_err = NULL;
>>> + bitmaps = qcow2_get_bitmap_info_list(bs, &local_err);
>>> + if (local_err != NULL) {
>>> + error_report_err(local_err);
>>> + }
>>
>> Ouch. Calling error_report_err() doesn't always work in QMP context;
>> better would be to plumb Error **errp back up to the caller, if
>> getting this specific information can fail and we want the overall
>> query-block to fail. Or, we could decide that failure to get bitmap
>> info is non-fatal, and that it was just a best-effort attempt to get
>> more info, where we then ignore the failure, rather than trying to
>> report it incorrectly.
> In my test environment, error_report_err() and warn_report_err() both
> cause printing an error message as the only output of 'qemu-img info'
> command.
Eric is right, using error_report() & friends in QMP context is almost
always wrong. Here's the issue in a bit more detail. Consider QMP
command query-block. Reverse call stack:
qmp_query_block()
bdrv_query_info()
bdrv_block_device_info()
bdrv_query_image_info()
bdrv_get_specific_info()
qcow2_get_specific_info()
The error you report in qcow2_get_specific_info() gets reported to
stderr. It is *not* reported to the QMP client. Why is that a good
idea?
[...]
Re: [Qemu-devel] [PATCH] cpus.c: Fix race condition in cpu_stop_current()
On Mon, 10 Dec 2018 at 12:15, Alex Bennée wrote: > Peter Maydell writes: > > though I might actually have meant pause_all_vcpus(). > > (For pause_all_vcpus() I think the correct thing is to > > fix the hw/i386/kvmvapic.c code to work in some other way, > > and then assert that pause_all_vcpus() is never called from > > the VCPU thread.) > > I thought we had already fixed this, but it is yet another case. See the > patch_instruction code in the same file. The only niggle is ensuring > that either the helper is called from last instruction in the block or > forcing a cpu_exit after queuing the work. Note that the call to pause_all_vcpus() is inside an "if (kvm_enabled())" so it doesn't matter what the TCG code does in the way of dividing code up into blocks. (Though the comment suggests that making it work in TCG might be nice in theory.) thanks -- PMM
Re: [Qemu-devel] [qemu-s390x] [PATCH] hw/s390x/virtio-ccw.c: Don't take address of fields in packed structs
On Mon, 10 Dec 2018 12:04:36 +
Peter Maydell wrote:
> Taking the address of a field in a packed struct is a bad idea, because
> it might not be actually aligned enough for that pointer type (and
> thus cause a crash on dereference on some host architectures). Newer
> versions of clang warn about this. Avoid the bug by not using the
> "modify in place" byte swapping functions.
>
> Patch produced with scripts/coccinelle/inplace-byteswaps.cocci
> (with a couple of long lines manually wrapped).
>
> Signed-off-by: Peter Maydell
Reviewed-by: Halil Pasic
I wonder if packed was a good idea in the first place. @Connie: Do you
have an opinion on this?
Regards,
Halil
> ---
> s390 also has some warnings in hw/s390x/css.c which are going to
> be harder to fix, relating to taking the address of the 'pmcw' and
> 'scsw' fields in the SCHUB struct...
>
> hw/s390x/virtio-ccw.c | 42 ++
> 1 file changed, 22 insertions(+), 20 deletions(-)
>
> diff --git a/hw/s390x/virtio-ccw.c b/hw/s390x/virtio-ccw.c
> index 212b3d3dead..c2b78c8e9b1 100644
> --- a/hw/s390x/virtio-ccw.c
> +++ b/hw/s390x/virtio-ccw.c
> @@ -287,18 +287,18 @@ static int virtio_ccw_handle_set_vq(SubchDev *sch, CCW1
> ccw, bool check_len,
> }
> if (is_legacy) {
> ccw_dstream_read(&sch->cds, linfo);
> -be64_to_cpus(&linfo.queue);
> -be32_to_cpus(&linfo.align);
> -be16_to_cpus(&linfo.index);
> -be16_to_cpus(&linfo.num);
> +linfo.queue = be64_to_cpu(linfo.queue);
> +linfo.align = be32_to_cpu(linfo.align);
> +linfo.index = be16_to_cpu(linfo.index);
> +linfo.num = be16_to_cpu(linfo.num);
> ret = virtio_ccw_set_vqs(sch, NULL, &linfo);
> } else {
> ccw_dstream_read(&sch->cds, info);
> -be64_to_cpus(&info.desc);
> -be16_to_cpus(&info.index);
> -be16_to_cpus(&info.num);
> -be64_to_cpus(&info.avail);
> -be64_to_cpus(&info.used);
> +info.desc = be64_to_cpu(info.desc);
> +info.index = be16_to_cpu(info.index);
> +info.num = be16_to_cpu(info.num);
> +info.avail = be64_to_cpu(info.avail);
> +info.used = be64_to_cpu(info.used);
> ret = virtio_ccw_set_vqs(sch, &info, NULL);
> }
> sch->curr_status.scsw.count = 0;
> @@ -382,7 +382,7 @@ static int virtio_ccw_cb(SubchDev *sch, CCW1 ccw)
> features.features = 0;
> }
> ccw_dstream_rewind(&sch->cds);
> -cpu_to_le32s(&features.features);
> +features.features = cpu_to_le32(features.features);
> ccw_dstream_write(&sch->cds, features.features);
> sch->curr_status.scsw.count = ccw.count - sizeof(features);
> ret = 0;
> @@ -403,7 +403,7 @@ static int virtio_ccw_cb(SubchDev *sch, CCW1 ccw)
> ret = -EFAULT;
> } else {
> ccw_dstream_read(&sch->cds, features);
> -le32_to_cpus(&features.features);
> +features.features = le32_to_cpu(features.features);
> if (features.index == 0) {
> virtio_set_features(vdev,
> (vdev->guest_features &
> 0xULL) |
> @@ -546,7 +546,7 @@ static int virtio_ccw_cb(SubchDev *sch, CCW1 ccw)
> ret = -EFAULT;
> } else {
> ccw_dstream_read(&sch->cds, indicators);
> -be64_to_cpus(&indicators);
> +indicators = be64_to_cpu(indicators);
> dev->indicators = get_indicator(indicators, sizeof(uint64_t));
> sch->curr_status.scsw.count = ccw.count - sizeof(indicators);
> ret = 0;
> @@ -567,7 +567,7 @@ static int virtio_ccw_cb(SubchDev *sch, CCW1 ccw)
> ret = -EFAULT;
> } else {
> ccw_dstream_read(&sch->cds, indicators);
> -be64_to_cpus(&indicators);
> +indicators = be64_to_cpu(indicators);
> dev->indicators2 = get_indicator(indicators, sizeof(uint64_t));
> sch->curr_status.scsw.count = ccw.count - sizeof(indicators);
> ret = 0;
> @@ -588,14 +588,14 @@ static int virtio_ccw_cb(SubchDev *sch, CCW1 ccw)
> ret = -EFAULT;
> } else {
> ccw_dstream_read(&sch->cds, vq_config.index);
> -be16_to_cpus(&vq_config.index);
> +vq_config.index = be16_to_cpu(vq_config.index);
> if (vq_config.index >= VIRTIO_QUEUE_MAX) {
> ret = -EINVAL;
> break;
> }
> vq_config.num_max = virtio_queue_get_num(vdev,
> vq_config.index);
> -cpu_to_be16s(&vq_config.num_max);
> +vq_config.num_max = cpu_to_be16(vq_config.num_max);
> ccw_dstream_write(&sch->cds, vq_config.num_max);
> sch->curr_status.scsw.count = ccw.count - sizeof(vq_config);
>
Re: [Qemu-devel] [RFC PATCH 0/6] target/ppc: convert VMX instructions to use TCG vector operations
On Dec 7, 2018 9:59 AM, "Mark Cave-Ayland"
wrote:
>
> This patchset is an attempt at trying to improve the VMX (Altivec)
instruction
> performance by making use of the new TCG vector operations where possible.
>
Hello, Mark.
I just want to say that I support these efforts. Very interesting, it can
bring significant improvements for multimedia intensive emulations. But,
even more important, we can see new tcg vector interface in action,
possibly suggest improvements, extensions.
I would just like to ask you to add some performance comparisons, if
possible. Very simple tests would be sufficient.
Thanks again for this series!
Aleksandar
> In order to use TCG vector operations, the registers must be accessible
from cpu_env
> whilst currently they are accessed via arrays of static TCG globals.
Patches 1-3
> are therefore mechanical patches which introduce access helpers for FPR,
AVR and VSR
> registers using the supplied TCGv_i64 parameter.
>
> Once this is done, patch 4 enables us to remove the static TCG global
arrays and updates
> the access helpers to read/write to the relevant fields in cpu_env
directly.
>
> The final patches 5 and 6 convert the VMX logical instructions and
addition/subtraction
> instructions respectively over to the TCG vector operations.
>
> NOTE: there are a lot of instructions that cannot (yet) be optimised to
use TCG vector
> operations, however it struck me that there may be some potential for
converting
> saturating add/sub and cmp instructions if there were a mechanism to
return a set of
> flags indicating the result of the saturation/comparison.
>
> Finally thanks to Richard for taking the time to answer some of my
(mostly beginner)
> questions related to TCG.
>
> Signed-off-by: Mark Cave-Ayland
>
>
> Mark Cave-Ayland (6):
> target/ppc: introduce get_fpr() and set_fpr() helpers for FP register
> access
> target/ppc: introduce get_avr64() and set_avr64() helpers for VMX
> register access
> target/ppc: introduce get_cpu_vsr{l,h}() and set_cpu_vsr{l,h}()
> helpers for VSR register access
> target/ppc: switch FPR, VMX and VSX helpers to access data directly
> from cpu_env
> target/ppc: convert VMX logical instructions to use vector operations
> target/ppc: convert vaddu[b,h,w,d] and vsubu[b,h,w,d] over to use
> vector operations
>
> target/ppc/helper.h | 8 -
> target/ppc/int_helper.c | 7 -
> target/ppc/translate.c | 72 ++--
> target/ppc/translate/fp-impl.inc.c | 492 ++-
> target/ppc/translate/vmx-impl.inc.c | 182 ++---
> target/ppc/translate/vsx-impl.inc.c | 782
++--
> 6 files changed, 1110 insertions(+), 433 deletions(-)
>
> --
> 2.11.0
>
>
Re: [Qemu-devel] Guests are crashing on startup, seem related to usb-audio
On Mon, Dec 10, 2018 at 12:11:09PM +, Leonardo Soares Müller wrote:
> Hi, I did not save that Mageia 7 data as I was unaware I could do this.
> The data below is from another crash with openSUSE Leap, this time I
> saved this backtrace with generate-core-file.
> On #4 it shows:
> $2 = {pid = 225, id = 2027203168, ep = 0x57e46520, stream = 0, iov =
> {iov = 0x7fffc418d190, niov = 0, nalloc = 1, size = 0}, parameter = 0,
> short_not_ok = false, int_req = false, status = 0, actual_length = 0,
> state = USB_PACKET_SETUP, combined = 0x0, queue = {tqe_next = 0x0,
> tqe_prev = 0x0}, combined_entry = {tqe_next = 0x0, tqe_prev = 0x0}}
Hmm, zero-length usb package. Should be 192 bytes ...
Does anything change with
https://git.kraxel.org/cgit/qemu/log/?h=sirius/usb-audio-crash ?
thanks,
Gerd
Re: [Qemu-devel] [RFC PATCH v2 3/3] target/ppc: support single stepping with KVM HV
David Gibson writes:
>> >> +if (arch_info->address == trace_handler_addr) {
>> >> +cpu_synchronize_state(cs);
>> >> +kvm_remove_breakpoint(cs, trace_handler_addr, 4,
>> >> GDB_BREAKPOINT_SW);
>> >> +
>> >> +cpu_memory_rw_debug(cs, env->spr[SPR_SRR0] - 4, (uint8_t *)&insn,
>> >> +sizeof(insn), 0);
>> >> +
>> >> +/* If the last instruction was a mfmsr, make sure that the
>> >> + * MSR_SE bit is not set to avoid the guest kernel knowing
>> >> + * that it is being single-stepped */
>> >> +if (extract32(insn, 26, 6) == 31 && extract32(insn, 1, 10) ==
>> >> 83) {
>> >> +reg = extract32(insn, 21, 5);
>> >> +env->gpr[reg] &= ~(1ULL << MSR_SE);
>> >> +}
>> >
>> > Hm. What happens if both qemu and the guest itself attempt to single
>> > step at the same time? How do you distinguish between the cases?
>>
>> There is currently no distinction being made.
>
> This seems incorrect to me. Basically you're restoring !MSR_SE
> unconditionaly when you finish the hypervisor side step, which might
> not be correct if the guest is also attempting to single step itself.
> AFAICT it should be possible to track what the guest thinks is the
> value of MSR_SE and restore that.
I was skeptical of being able to do both single steps at the same time
but I found a way to reproduce it by stepping over an rfid when
SRR1_SE is already 1.
> If both hypervisor and guest
> attempt to single step, I'd expect to have the hypervisor trap first,
> then return to the guest's single step vector.
With the fix you suggest above, QEMU will be able to single step the
interrupt handler during the guest's single step. That means I'll have
to restore the SRRs as well so that the handler returns to the correct
place.
>> I could do the latter, if you prefer.
>
> I think that's better - I don't think it's safe to assume that you're
> *not* synchronized with KVM.
Ok, that's better indeed.
Thanks for the comments, I'll prepare another version with the
appropriate corrections.
Re: [Qemu-devel] [PATCH] hw/s390x/virtio-ccw.c: Don't take address of fields in packed structs
On 12/10/18 1:04 PM, Peter Maydell wrote:
> Taking the address of a field in a packed struct is a bad idea, because
> it might not be actually aligned enough for that pointer type (and
> thus cause a crash on dereference on some host architectures). Newer
> versions of clang warn about this. Avoid the bug by not using the
> "modify in place" byte swapping functions.
>
> Patch produced with scripts/coccinelle/inplace-byteswaps.cocci
> (with a couple of long lines manually wrapped).
>
> Signed-off-by: Peter Maydell
Reviewed-by: Philippe Mathieu-Daudé
> ---
> s390 also has some warnings in hw/s390x/css.c which are going to
> be harder to fix, relating to taking the address of the 'pmcw' and
> 'scsw' fields in the SCHUB struct...
>
> hw/s390x/virtio-ccw.c | 42 ++
> 1 file changed, 22 insertions(+), 20 deletions(-)
>
> diff --git a/hw/s390x/virtio-ccw.c b/hw/s390x/virtio-ccw.c
> index 212b3d3dead..c2b78c8e9b1 100644
> --- a/hw/s390x/virtio-ccw.c
> +++ b/hw/s390x/virtio-ccw.c
> @@ -287,18 +287,18 @@ static int virtio_ccw_handle_set_vq(SubchDev *sch, CCW1
> ccw, bool check_len,
> }
> if (is_legacy) {
> ccw_dstream_read(&sch->cds, linfo);
> -be64_to_cpus(&linfo.queue);
> -be32_to_cpus(&linfo.align);
> -be16_to_cpus(&linfo.index);
> -be16_to_cpus(&linfo.num);
> +linfo.queue = be64_to_cpu(linfo.queue);
> +linfo.align = be32_to_cpu(linfo.align);
> +linfo.index = be16_to_cpu(linfo.index);
> +linfo.num = be16_to_cpu(linfo.num);
> ret = virtio_ccw_set_vqs(sch, NULL, &linfo);
> } else {
> ccw_dstream_read(&sch->cds, info);
> -be64_to_cpus(&info.desc);
> -be16_to_cpus(&info.index);
> -be16_to_cpus(&info.num);
> -be64_to_cpus(&info.avail);
> -be64_to_cpus(&info.used);
> +info.desc = be64_to_cpu(info.desc);
> +info.index = be16_to_cpu(info.index);
> +info.num = be16_to_cpu(info.num);
> +info.avail = be64_to_cpu(info.avail);
> +info.used = be64_to_cpu(info.used);
> ret = virtio_ccw_set_vqs(sch, &info, NULL);
> }
> sch->curr_status.scsw.count = 0;
> @@ -382,7 +382,7 @@ static int virtio_ccw_cb(SubchDev *sch, CCW1 ccw)
> features.features = 0;
> }
> ccw_dstream_rewind(&sch->cds);
> -cpu_to_le32s(&features.features);
> +features.features = cpu_to_le32(features.features);
> ccw_dstream_write(&sch->cds, features.features);
> sch->curr_status.scsw.count = ccw.count - sizeof(features);
> ret = 0;
> @@ -403,7 +403,7 @@ static int virtio_ccw_cb(SubchDev *sch, CCW1 ccw)
> ret = -EFAULT;
> } else {
> ccw_dstream_read(&sch->cds, features);
> -le32_to_cpus(&features.features);
> +features.features = le32_to_cpu(features.features);
> if (features.index == 0) {
> virtio_set_features(vdev,
> (vdev->guest_features &
> 0xULL) |
> @@ -546,7 +546,7 @@ static int virtio_ccw_cb(SubchDev *sch, CCW1 ccw)
> ret = -EFAULT;
> } else {
> ccw_dstream_read(&sch->cds, indicators);
> -be64_to_cpus(&indicators);
> +indicators = be64_to_cpu(indicators);
> dev->indicators = get_indicator(indicators, sizeof(uint64_t));
> sch->curr_status.scsw.count = ccw.count - sizeof(indicators);
> ret = 0;
> @@ -567,7 +567,7 @@ static int virtio_ccw_cb(SubchDev *sch, CCW1 ccw)
> ret = -EFAULT;
> } else {
> ccw_dstream_read(&sch->cds, indicators);
> -be64_to_cpus(&indicators);
> +indicators = be64_to_cpu(indicators);
> dev->indicators2 = get_indicator(indicators, sizeof(uint64_t));
> sch->curr_status.scsw.count = ccw.count - sizeof(indicators);
> ret = 0;
> @@ -588,14 +588,14 @@ static int virtio_ccw_cb(SubchDev *sch, CCW1 ccw)
> ret = -EFAULT;
> } else {
> ccw_dstream_read(&sch->cds, vq_config.index);
> -be16_to_cpus(&vq_config.index);
> +vq_config.index = be16_to_cpu(vq_config.index);
> if (vq_config.index >= VIRTIO_QUEUE_MAX) {
> ret = -EINVAL;
> break;
> }
> vq_config.num_max = virtio_queue_get_num(vdev,
> vq_config.index);
> -cpu_to_be16s(&vq_config.num_max);
> +vq_config.num_max = cpu_to_be16(vq_config.num_max);
> ccw_dstream_write(&sch->cds, vq_config.num_max);
> sch->curr_status.scsw.count = ccw.count - sizeof(vq_config);
> ret = 0;
> @@ -621,9 +621,11 @@ static int virtio_ccw_cb(SubchDev *sch, CCW1 ccw)
> if (ccw_dstream_
Re: [Qemu-devel] [PATCH] target/i386/kvm.c: Don't mark cpuid_data as QEMU_PACKED
On 12/10/18 12:46 PM, Peter Maydell wrote:
> clang complains about taking the address of a packed
> member of a struct:
>
> target/i386/kvm.c:1245:27: warning: taking address of packed member 'cpuid'
> of class or structure '' may result in an unaligned pointer value
> [-Waddress-of-packed-member]
> c = cpuid_find_entry(&cpuid_data.cpuid, 1, 0);
> ^~~~
> target/i386/kvm.c:1297:31: warning: taking address of packed member 'cpuid'
> of class or structure '' may result in an unaligned pointer value
> [-Waddress-of-packed-member]
> c = cpuid_find_entry(&cpuid_data.cpuid, kvm_base, 0);
> ^~~~
>
> The kernel's definitions of struct kvm_cpuid2 and struct
> kvm_cpuid_entry2 are carefully set up with padding fields
> so that there is no between-struct padding anyway, so
> the QEMU_PACKED annotation is unnecessary and might result
> in the compiler generating worse code. Drop it, and instead
> assert at build time that there is no stray padding.
>
> Signed-off-by: Peter Maydell
Reviewed-by: Philippe Mathieu-Daudé
> ---
> target/i386/kvm.c | 10 +-
> 1 file changed, 9 insertions(+), 1 deletion(-)
>
> diff --git a/target/i386/kvm.c b/target/i386/kvm.c
> index b2401d13ea7..739cf8c8ea1 100644
> --- a/target/i386/kvm.c
> +++ b/target/i386/kvm.c
> @@ -864,7 +864,15 @@ int kvm_arch_init_vcpu(CPUState *cs)
> struct {
> struct kvm_cpuid2 cpuid;
> struct kvm_cpuid_entry2 entries[KVM_MAX_CPUID_ENTRIES];
> -} QEMU_PACKED cpuid_data;
> +} cpuid_data;
> +/*
> + * The kernel defines these structs with padding fields so there
> + * should be no extra padding in our cpuid_data struct.
> + */
> +QEMU_BUILD_BUG_ON(sizeof(cpuid_data) !=
> + sizeof(struct kvm_cpuid2) +
> + sizeof(struct kvm_cpuid_entry2) *
> KVM_MAX_CPUID_ENTRIES);
> +
> X86CPU *cpu = X86_CPU(cs);
> CPUX86State *env = &cpu->env;
> uint32_t limit, i, j, cpuid_i;
>
Re: [Qemu-devel] [PATCH] blk: postpone request execution on a context protected with "drained section"
Am 10.12.2018 um 13:14 hat Denis Plotnikov geschrieben:
> >> @@ -491,9 +506,17 @@ int64_t aio_compute_timeout(AioContext *ctx);
> >>*/
> >> static inline void aio_disable_external(AioContext *ctx)
> >> {
> >> +aio_context_acquire(ctx);
> >> atomic_inc(&ctx->external_disable_cnt);
> >> +aio_context_release(ctx);
> >> }
> >
> > This acquire/release pair looks rather useless?
>
> I'm not sure that I understand everything correctly...
> but can a thread (context) try to disable external in another context?
Yes, that can happen. For example, think of bdrv_drain_all().
Kevin
Re: [Qemu-devel] Guests are crashing on startup, seem related to usb-audio
Hi, I did not save that Mageia 7 data as I was unaware I could do this. The data below is from another crash with openSUSE Leap, this time I saved this backtrace with generate-core-file. QEMU command line: env QEMU_AUDIO_ADC_VOICES=0 QEMU_AUDIO_DRV=pa \ QEMU_AUDIO_DAC_FIXED_FREQ=96000 \ QEMU_AUDIO_ADC_FIXED_FREQ=96000 \ QEMU_AUDIO_ADC_VOICES=0 \ gdb -ex "handle SIGUSR1 nostop nopass noprint" -ex "run" --args qemu-system-x86_64 \ -name "openSUSE Leap" -k pt-br -nodefaults -enable-kvm -cpu host -smp cores=2,threads=1 -m 2G \ -device qemu-xhci,id=xhcihub -device usb-audio,id=usbaudio,buffer=6144 \ -device virtio-tablet-pci,id=pcitablet -bios /usr/share/ovmf/OVMF.fd \ -device qxl-vga,xres=800,yres=600 -display gtk,gl=on \ -hda /home/usuario/.local/share/libvirt/images/opensuse_leap.qcow2 \ -monitor vc -serial vc \ -machine kernel_irqchip=on -global PIIX4_PM.disable_s3=1 -global PIIX4_PM.disable_s4=1 -M pc,usb=true \ -netdev user,id=net0 -device e1000,netdev=net0,addr=8 gdb backtrace: #0 0x701cce97 in __GI_raise (sig=sig@entry=6) at ../sysdeps/unix/sysv/linux/raise.c:51 #1 0x701ce801 in __GI_abort () at abort.c:79 #2 0x701be39a in __assert_fail_base (fmt=0x7fffd403e202 "%s%s%s:%u: %s%sAssertiva “%s” falhou.\n%n", assertion=assertion@entry=0x55fb8738 "p->actual_length + bytes <= iov->size", file=file@entry=0x55fb8456 "hw/usb/core.c", line=line@entry=592, function=function@entry=0x55fb8980 <__PRETTY_FUNCTION__.26351> "usb_packet_copy") at assert.c:92 #3 0x701be412 in __GI___assert_fail (assertion=0x55fb8738 "p->actual_length + bytes <= iov->size", file=0x55fb8456 "hw/usb/core.c", line=592, function=0x55fb8980 <__PRETTY_FUNCTION__.26351> "usb_packet_copy") at assert.c:101 #4 0x55bd5ed7 in usb_packet_copy (p=0x7fffc4174128, ptr=0x7fffb801e390, bytes=192) at hw/usb/core.c:592 #5 0x55c024d8 in streambuf_put (buf=0x57e468a0, p=0x7fffc4174128) at hw/usb/dev-audio.c:325 #6 0x55c02d78 in usb_audio_handle_dataout (s=0x57e451b0, p=0x7fffc4174128) at hw/usb/dev-audio.c:596 #7 0x55c02e16 in usb_audio_handle_data (dev=0x57e451b0, p=0x7fffc4174128) at hw/usb/dev-audio.c:608 #8 0x55bd7c39 in usb_device_handle_data (dev=0x57e451b0, p=0x7fffc4174128) at hw/usb/bus.c:184 #9 0x55bd54a9 in usb_process_one (p=0x7fffc4174128) at hw/usb/core.c:388 #10 0x55bd5668 in usb_handle_packet (dev=0x57e451b0, p=0x7fffc4174128) at hw/usb/core.c:420 #11 0x55bf6d8e in xhci_submit (xhci=0x7fffcd538010, xfer=0x7fffc4174120, epctx=0x7fffc4172f40) at hw/usb/hcd-xhci.c:1819 #12 0x55bf6df6 in xhci_fire_transfer (xhci=0x7fffcd538010, xfer=0x7fffc4174120, epctx=0x7fffc4172f40) at hw/usb/hcd-xhci.c:1828 #13 0x55bf73eb in xhci_kick_epctx (epctx=0x7fffc4172f40, streamid=0) at hw/usb/hcd-xhci.c:1969 #14 0x55bf6eef in xhci_kick_ep (xhci=0x7fffcd538010, slotid=1, epid=2, streamid=0) at hw/usb/hcd-xhci.c:1853 #15 0x55bfa0ac in xhci_doorbell_write (ptr=0x7fffcd538010, reg=1, val=2, size=4) at hw/usb/hcd-xhci.c:3125 #16 0x5587f44e in memory_region_write_accessor (mr=0x7fffcd538d60, addr=4, value=0x7fffceeb80b8, size=4, shift=0, mask=4294967295, attrs=...) at /home/usuario/Documentos/qemu/memory.c:504 #17 0x5587f65e in access_with_adjusted_size (addr=4, value=0x7fffceeb80b8, size=4, access_size_min=1, access_size_max=4, access_fn= 0x5587f365 , mr=0x7fffcd538d60, attrs=...) at /home/usuario/Documentos/qemu/memory.c:570 #18 0x55882359 in memory_region_dispatch_write (mr=0x7fffcd538d60, addr=4, data=2, size=4, attrs=...) at /home/usuario/Documentos/qemu/memory.c:1452 #19 0x5581d359 in flatview_write_continue (fv=0x7fffc4188bc0, addr=34359762948, attrs=..., buf=0x77ff3028 "\002", len=4, addr1=4, l=4, mr=0x7fffcd538d60) at /home/usuario/Documentos/qemu/exec.c:3233 #20 0x5581d4a3 in flatview_write (fv=0x7fffc4188bc0, addr=34359762948, attrs=..., buf=0x77ff3028 "\002", len=4) at /home/usuario/Documentos/qemu/exec.c:3272 #21 0x5581d7a9 in address_space_write (as=0x567d6460 , addr=34359762948, attrs=..., buf=0x77ff3028 "\002", len=4) at /home/usuario/Documentos/qemu/exec.c:3362 #22 0x5581d7fa in address_space_rw (as=0x567d6460 , addr=34359762948, attrs=..., buf=0x77ff3028 "\002", len=4, is_write=true) at /home/usuario/Documentos/qemu/exec.c:3373 #23 0x5589ea33 in kvm_cpu_exec (cpu=0x56b9ddf0) at /home/usuario/Documentos/qemu/accel/kvm/kvm-all.c:2031 #24 0x5586453b in qemu_kvm_cpu_thread_fn (arg=0x56b9ddf0) at /home/usuario/Documentos/qemu/cpus.c:1281 #25 0x55e11d07 in qemu_thread_start (args=0x56bbe520) at util/qemu-thread-posix.c:498 #26 0x705866db in start_thread (arg=0x7fffceebb700) at pthread_create.c:463 #27 0x702af88f in clone () at ../sysdeps/unix/sysv/linux/x86_64/clone.S:95 On #3 it outputs: No symbol "p" in current context. On #4 it shows: $2
Re: [Qemu-devel] [PATCH] blk: postpone request execution on a context protected with "drained section"
On 07.12.2018 15:26, Kevin Wolf wrote:
> Am 05.12.2018 um 13:23 hat Denis Plotnikov geschrieben:
>> At the time, the "drained section" doesn't protect Block Driver State
>> from the requests appearing in the vCPU threads.
>> This could lead to the data loss because of request coming to
>> an unexpected BDS.
>>
>> For example, when a request comes to ide controller from the guest,
>> the controller creates a request coroutine and executes the coroutine
>> in the vCPU thread. If another thread(iothread) has entered the
>> "drained section" on a BDS with bdrv_drained_begin, which protects
>> BDS' AioContext from external requests, and released the AioContext
>> because of finishing some coroutine by the moment of the request
>> appearing at the ide controller, the controller acquires the AioContext
>> and executes its request without any respect to the entered
>> "drained section" producing any kinds of data inconsistency.
>>
>> The patch prevents this case by putting requests from external threads to
>> the queue on AioContext while the context is protected for external requests
>> and executes those requests later on the external requests protection
>> removing.
In general, I agree with the comments and going to make changes in the
patches accordingly.
Also, I'd like to ask a question below
>>
>> Also, the patch marks requests generated in a vCPU thread as external ones
>> to make use of the request postponing.
>>
>> How to reproduce:
>> 1. start vm with an ide disk and a linux guest
>> 2. in the guest run: dd if=... of=... bs=4K count=10 oflag=direct
>> 3. (qemu) drive_mirror "disk-name"
>> 4. wait until block job can receive block_job_complete
>> 5. (qemu) block_job_complete "disk-name"
>> 6. blk_aio_p[read|write]v may appear in vCPU context (here is the race)
>>
>> Signed-off-by: Denis Plotnikov
>
> This is getting closer, but I'd like to see two more major changes:
>
>> diff --git a/include/block/aio.h b/include/block/aio.h
>> index 0ca25dfec6..8512bda44e 100644
>> --- a/include/block/aio.h
>> +++ b/include/block/aio.h
>> @@ -19,6 +19,7 @@
>> #include "qemu/event_notifier.h"
>> #include "qemu/thread.h"
>> #include "qemu/timer.h"
>> +#include "qemu/coroutine.h"
>>
>> typedef struct BlockAIOCB BlockAIOCB;
>> typedef void BlockCompletionFunc(void *opaque, int ret);
>> @@ -130,6 +131,11 @@ struct AioContext {
>> QEMUTimerListGroup tlg;
>>
>> int external_disable_cnt;
>> +/* Queue to store the requests coming when the context is disabled for
>> + * external requests.
>> + * Don't use a separate lock for protection relying the context lock
>> + */
>> +CoQueue postponed_reqs;
>
> Why involve the AioContext at all? This could all be kept at the
> BlockBackend level without extending the layering violation that
> aio_disable_external() is.
>
> BlockBackends get notified when their root node is drained, so hooking
> things up there should be as easy, if not even easier than in
> AioContext.
>
>> /* Number of AioHandlers without .io_poll() */
>> int poll_disable_cnt;
>> @@ -483,6 +489,15 @@ static inline void aio_timer_init(AioContext *ctx,
>>*/
>> int64_t aio_compute_timeout(AioContext *ctx);
>>
>> +/**
>> + * aio_co_enter:
>> + * @ctx: the context to run the coroutine
>> + * @co: the coroutine to run
>> + *
>> + * Enter a coroutine in the specified AioContext.
>> + */
>> +void aio_co_enter(AioContext *ctx, struct Coroutine *co);
>> +
>> /**
>>* aio_disable_external:
>>* @ctx: the aio context
>> @@ -491,9 +506,17 @@ int64_t aio_compute_timeout(AioContext *ctx);
>>*/
>> static inline void aio_disable_external(AioContext *ctx)
>> {
>> +aio_context_acquire(ctx);
>> atomic_inc(&ctx->external_disable_cnt);
>> +aio_context_release(ctx);
>> }
>
> This acquire/release pair looks rather useless?
I'm not sure that I understand everything correctly...
but can a thread (context) try to disable external in another context?
>
>> +static void run_postponed_co(void *opaque)
>> +{
>> +AioContext *ctx = (AioContext *) opaque;
>> +
>> +qemu_co_queue_restart_all(&ctx->postponed_reqs);
>> +}
>> /**
>>* aio_enable_external:
>>* @ctx: the aio context
>> @@ -504,12 +527,17 @@ static inline void aio_enable_external(AioContext *ctx)
>> {
>> int old;
>>
>> +aio_context_acquire(ctx);
>> old = atomic_fetch_dec(&ctx->external_disable_cnt);
>> assert(old > 0);
>> if (old == 1) {
>> +Coroutine *co = qemu_coroutine_create(run_postponed_co, ctx);
>> +aio_co_enter(ctx, co);
>> +
>> /* Kick event loop so it re-arms file descriptors */
>> aio_notify(ctx);
>> }
>> +aio_context_release(ctx);
>> }
>>
>> /**
>> @@ -564,15 +592,6 @@ void aio_co_schedule(AioContext *ctx, struct Coroutine
>> *co);
>>*/
>> void aio_co_wake(struct Coroutine *co);
>>
>> -/**
>> - * aio_co_enter:
>> - * @ctx: the context to run the coroutine
>>
Re: [Qemu-devel] [PATCH v3 3/3] hw/s390x/ioinst: Fix alignment problem in struct SubchDev
On Thu, 27 Sep 2018 at 09:25, Thomas Huth wrote:
>
> struct SubchDev embeds several other structures which are marked with
> QEMU_PACKED. This causes the compiler to not care for proper alignment
> of these structures. When we later pass around pointers to the unaligned
> struct members during migration, this causes problems on host architectures
> like Sparc that can not do unaligned memory access.
>
> Most of the structs in ioinst.h are naturally aligned, so we can fix
> most of the problem by removing the QEMU_PACKED statements (and use
> QEMU_BUILD_BUG_MSG() statements instead to make sure that there is no
> padding). However, for the struct SCHIB, we have to keep the QEMU_PACKED
> since the compiler adds some padding here otherwise. Move this struct
> to the beginning of struct SubchDev instead to fix the alignment problem
> here, too.
Unfortunately clang does not like the struct SCHIB being still
marked packed:
/home/petmay01/linaro/qemu-from-laptop/qemu/hw/s390x/css.c:1294:25:
warning: taking address of packed member 'pmcw' of class or structure
'SCHIB' may result in an unaligned pointer value
[-Waddress-of-packed-member]
copy_pmcw_to_guest(&dest->pmcw, &src->pmcw);
^~
/home/petmay01/linaro/qemu-from-laptop/qemu/hw/s390x/css.c:1294:38:
warning: taking address of packed member 'pmcw' of class or structure
'SCHIB' may result in an unaligned pointer value
[-Waddress-of-packed-member]
copy_pmcw_to_guest(&dest->pmcw, &src->pmcw);
^
/home/petmay01/linaro/qemu-from-laptop/qemu/hw/s390x/css.c:1295:25:
warning: taking address of packed member 'scsw' of class or structure
'SCHIB' may result in an unaligned pointer value
[-Waddress-of-packed-member]
copy_scsw_to_guest(&dest->scsw, &src->scsw);
^~
/home/petmay01/linaro/qemu-from-laptop/qemu/hw/s390x/css.c:1295:38:
warning: taking address of packed member 'scsw' of class or structure
'SCHIB' may result in an unaligned pointer value
[-Waddress-of-packed-member]
copy_scsw_to_guest(&dest->scsw, &src->scsw);
^
/home/petmay01/linaro/qemu-from-laptop/qemu/hw/s390x/css.c:1343:27:
warning: taking address of packed member 'pmcw' of class or structure
'SCHIB' may result in an unaligned pointer value
[-Waddress-of-packed-member]
copy_pmcw_from_guest(&dest->pmcw, &src->pmcw);
^~
/home/petmay01/linaro/qemu-from-laptop/qemu/hw/s390x/css.c:1343:40:
warning: taking address of packed member 'pmcw' of class or structure
'SCHIB' may result in an unaligned pointer value
[-Waddress-of-packed-member]
copy_pmcw_from_guest(&dest->pmcw, &src->pmcw);
^
/home/petmay01/linaro/qemu-from-laptop/qemu/hw/s390x/css.c:1344:27:
warning: taking address of packed member 'scsw' of class or structure
'SCHIB' may result in an unaligned pointer value
[-Waddress-of-packed-member]
copy_scsw_from_guest(&dest->scsw, &src->scsw);
^~
/home/petmay01/linaro/qemu-from-laptop/qemu/hw/s390x/css.c:1344:40:
warning: taking address of packed member 'scsw' of class or structure
'SCHIB' may result in an unaligned pointer value
[-Waddress-of-packed-member]
copy_scsw_from_guest(&dest->scsw, &src->scsw);
^
Not sure how best to address this. A couple of ideas that I had:
(1) make the 'uint64_t mba' field in the SCHIB struct into
two uint32_t fields, adjusting all the code which needs
to access it accordingly; then we could drop the packed
annotation from the struct
(2) have the guts of copy_{pmcw,scsw}_{to,from}_guest() be
macros, so we can do them inline in the copy_schib_{to,from}_guest()
function and thus operate directly on src->pmcw.foo &c
fields rather than ever having to take the address of any
of the fields in src or dest
thanks
-- PMM
Re: [Qemu-devel] [PATCH v4] qemu-img info lists bitmap directory entries
On 07.12.2018 19:20, Eric Blake wrote:
> On 12/7/18 4:00 AM, Andrey Shinkevich wrote:
>> In the 'Format specific information' section of the 'qemu-img info'
>> command output, the supplemental information about existing QCOW2
>> bitmaps will be shown, such as a bitmap name, flags and granularity:
>>
>> image: /vz/vmprivate/VM1/harddisk.hdd
>> file format: qcow2
>> virtual size: 64G (68719476736 bytes)
>> disk size: 3.0M
>> cluster_size: 1048576
>> Format specific information:
>> compat: 1.1
>> lazy refcounts: true
>> bitmaps:
>> [0]:
>> flags:
>> [0]: in-use
>> [1]: auto
>> name: back-up1
>> unknown flags: 4
>
> I'm guessing you doctored an image in a hex-editor to get this
> particular output?
Actually, I hardcoded unspecified flags just to sample the output with them.
>
>
>> granularity: 65536
>> [1]:
>> flags:
>> [0]: in-use
>> [1]: auto
>> name: back-up2
>> unknown flags: 8
>> granularity: 65536
>> refcount bits: 16
>> corrupt: false
>>
>> Signed-off-by: Andrey Shinkevich
>> Reviewed-by: Vladimir Sementsov-Ogievskiy
>> ---
>> v4:
>> Unknown flags are checked with the mask BME_RESERVED_FLAGS.
>> The code minor refactoring was made.
>>
>
>> block/qcow2-bitmap.c | 56
>>
>> block/qcow2.c | 8
>> block/qcow2.h | 2 ++
>> qapi/block-core.json | 40 -
>> 4 files changed, 105 insertions(+), 1 deletion(-)
>
> I'm assuming John will merge this as a bitmap-related patch; make sure
> he is in cc if you send a v5 (adding now).
Well noted, thank you!
>
>> +++ b/block/qcow2.c
>> @@ -4270,6 +4270,12 @@ static ImageInfoSpecific
>> *qcow2_get_specific_info(BlockDriverState *bs)
>> .refcount_bits = s->refcount_bits,
>> };
>> } else if (s->qcow_version == 3) {
>> + Qcow2BitmapInfoList *bitmaps;
>> + Error *local_err = NULL;
>> + bitmaps = qcow2_get_bitmap_info_list(bs, &local_err);
>> + if (local_err != NULL) {
>> + error_report_err(local_err);
>> + }
>
> Ouch. Calling error_report_err() doesn't always work in QMP context;
> better would be to plumb Error **errp back up to the caller, if
> getting this specific information can fail and we want the overall
> query-block to fail. Or, we could decide that failure to get bitmap
> info is non-fatal, and that it was just a best-effort attempt to get
> more info, where we then ignore the failure, rather than trying to
> report it incorrectly.
In my test environment, error_report_err() and warn_report_err() both
cause printing an error message as the only output of 'qemu-img info'
command.
>
>
>> +++ b/qapi/block-core.json
>> @@ -69,6 +69,8 @@
>> # @encrypt: details about encryption parameters; only set if image
>> # is encrypted (since 2.10)
>> #
>> +# @bitmaps: list of qcow2 bitmaps details (since 4.0)
>> +#
>> # Since: 1.7
>> ##
>> { 'struct': 'ImageInfoSpecificQCow2',
>> @@ -77,7 +79,8 @@
>> '*lazy-refcounts': 'bool',
>> '*corrupt': 'bool',
>> 'refcount-bits': 'int',
>> - '*encrypt': 'ImageInfoSpecificQCow2Encryption'
>> + '*encrypt': 'ImageInfoSpecificQCow2Encryption',
>> + '*bitmaps': ['Qcow2BitmapInfo']
>
> Hmm. You're omitting this field both if there are 0 bitmaps, and when
> it was a version 2 image. Is it worth including this field as a
> 0-length array when there are no bitmaps but when the image format is
> new enough to support them, or are we happy with the idea of only
> including the field when it has at least one bitmap? The difference is
> whether the calling app can explicitly learn that there are no bitmaps
> (0-length reply) vs. the ambiguity of omitting it from the reply
> (missing might mean no bitmaps, or an error in trying to report the
> bitmaps, or an older qemu that didn't know how to report bitmaps).
What if we add the information about printing birmaps to the QEMU-IMG
online manual and will omit it in the command output in case of no bitmap?
>
>
>> +##
>> +# @Qcow2BitmapInfo:
>> +#
>> +# Qcow2 bitmap information.
>> +#
>> +# @name: the name of the bitmap
>> +#
>> +# @granularity: granularity of the bitmap in bytes
>> +#
>> +# @flags: flags of the bitmap
>> +#
>> +# @unknown-flags: unspecified flags if detected
>> +#
>> +# Since: 4.0
>> +##
>> +{ 'struct': 'Qcow2BitmapInfo',
>> + 'data': {'name': 'str', 'granularity': 'uint32',
>> + 'flags': ['Qcow2BitmapInfoFlags'],
>> + '*unknown-flags': 'uint32' } }
>
> Here, you said flags will always be present, even if it is a 0-length
> array. Did you test the case where an on-disk bitmap has neither
> 'in-use' nor 'auto' set (where get_bitmap_info_flags() returns NULL)
> to ensure that it ind
Re: [Qemu-devel] [PATCH] cpus.c: Fix race condition in cpu_stop_current()
Peter Maydell writes:
> On Mon, 10 Dec 2018 at 11:06, Alex Bennée wrote:
>>
>>
>> Peter Maydell writes:
>> > We discussed this a little while back:
>> > https://lists.gnu.org/archive/html/qemu-devel/2018-10/msg00154.html
>> > and Jaap reported a bug which I suspect of being the same thing:
>> > https://lists.gnu.org/archive/html/qemu-discuss/2018-10/msg00014.html
>> >
>> > Annoyingly I have lost the test case that demonstrated this
>> > race, but I analysed it at the time and this should definitely
>> > fix it. I have opted not to try to address any of the other
>> > possible cleanup here (eg vm_stop() has a potential similar
>> > race if called from a vcpu thread I suspect), since it gets
>> > pretty tangled.
>> >
>> > Jaap: could you test whether this patch fixes the issue you
>> > were seeing, please?
>> > ---
>> > cpus.c | 3 ++-
>> > 1 file changed, 2 insertions(+), 1 deletion(-)
>> >
>> > diff --git a/cpus.c b/cpus.c
>> > index 0ddeeefc14f..b09b7027126 100644
>> > --- a/cpus.c
>> > +++ b/cpus.c
>> > @@ -2100,7 +2100,8 @@ void qemu_init_vcpu(CPUState *cpu)
>> > void cpu_stop_current(void)
>> > {
>> > if (current_cpu) {
>> > -qemu_cpu_stop(current_cpu, true);
>> > +current_cpu->stop = true;
>> > +cpu_exit(current_cpu);
>>
>> Should the FIXME in vm_stop also be fixed?
>>
>> /*
>> * FIXME: should not return to device code in case
>> * vm_stop() has been requested.
>> */
>> cpu_stop_current();
>> return 0;
>
> This is one of the things I had in mind with:
>> > I have opted not to try to address any of the other
>> > possible cleanup here (eg vm_stop() has a potential similar
>> > race if called from a vcpu thread I suspect), since it gets
>> > pretty tangled.
>
> though I might actually have meant pause_all_vcpus().
> (For pause_all_vcpus() I think the correct thing is to
> fix the hw/i386/kvmvapic.c code to work in some other way,
> and then assert that pause_all_vcpus() is never called from
> the VCPU thread.)
I thought we had already fixed this, but it is yet another case. See the
patch_instruction code in the same file. The only niggle is ensuring
that either the helper is called from last instruction in the block or
forcing a cpu_exit after queuing the work.
The i386 helpers are tricky because they seem to be very deeply nested
so its hard to be sure everything really is done.
But yes I agree that pause_all_vcpus() should be reserved for non-vCPU
contexts only.
> At any rate, this code is quite complicated, so I think
> it's worth just fixing this specific race without getting
> tangled up in everything else we could potentially refactor.
Fair enough.
>
> I'm not even sure how we would arrange for vm_stop() to
> avoid returning to device emulation code if it has been
> called from there -- I would favour instead changing/defining
> the semantics to be like the shutdown-request and reset-request
> where the device code expects that control will return but
> the VM stop happens at the next opportunity, ie as soon
> as the execution of the insn which wrote to the device
> register has completed.
Is there anyway we can assert we are in a helper that has caused all
globals to be saved before the call? Otherwise we need to make similar
guarantees that the ARM TLB flushes have that they are always the last
in a block of instructions.
prep_port0092_write in PPC seems to do a similar thing. Perhaps we need
to expose some common helpers for vcpus?
--
Alex Bennée
[Qemu-devel] [PATCH] hw/s390x/virtio-ccw.c: Don't take address of fields in packed structs
Taking the address of a field in a packed struct is a bad idea, because
it might not be actually aligned enough for that pointer type (and
thus cause a crash on dereference on some host architectures). Newer
versions of clang warn about this. Avoid the bug by not using the
"modify in place" byte swapping functions.
Patch produced with scripts/coccinelle/inplace-byteswaps.cocci
(with a couple of long lines manually wrapped).
Signed-off-by: Peter Maydell
---
s390 also has some warnings in hw/s390x/css.c which are going to
be harder to fix, relating to taking the address of the 'pmcw' and
'scsw' fields in the SCHUB struct...
hw/s390x/virtio-ccw.c | 42 ++
1 file changed, 22 insertions(+), 20 deletions(-)
diff --git a/hw/s390x/virtio-ccw.c b/hw/s390x/virtio-ccw.c
index 212b3d3dead..c2b78c8e9b1 100644
--- a/hw/s390x/virtio-ccw.c
+++ b/hw/s390x/virtio-ccw.c
@@ -287,18 +287,18 @@ static int virtio_ccw_handle_set_vq(SubchDev *sch, CCW1
ccw, bool check_len,
}
if (is_legacy) {
ccw_dstream_read(&sch->cds, linfo);
-be64_to_cpus(&linfo.queue);
-be32_to_cpus(&linfo.align);
-be16_to_cpus(&linfo.index);
-be16_to_cpus(&linfo.num);
+linfo.queue = be64_to_cpu(linfo.queue);
+linfo.align = be32_to_cpu(linfo.align);
+linfo.index = be16_to_cpu(linfo.index);
+linfo.num = be16_to_cpu(linfo.num);
ret = virtio_ccw_set_vqs(sch, NULL, &linfo);
} else {
ccw_dstream_read(&sch->cds, info);
-be64_to_cpus(&info.desc);
-be16_to_cpus(&info.index);
-be16_to_cpus(&info.num);
-be64_to_cpus(&info.avail);
-be64_to_cpus(&info.used);
+info.desc = be64_to_cpu(info.desc);
+info.index = be16_to_cpu(info.index);
+info.num = be16_to_cpu(info.num);
+info.avail = be64_to_cpu(info.avail);
+info.used = be64_to_cpu(info.used);
ret = virtio_ccw_set_vqs(sch, &info, NULL);
}
sch->curr_status.scsw.count = 0;
@@ -382,7 +382,7 @@ static int virtio_ccw_cb(SubchDev *sch, CCW1 ccw)
features.features = 0;
}
ccw_dstream_rewind(&sch->cds);
-cpu_to_le32s(&features.features);
+features.features = cpu_to_le32(features.features);
ccw_dstream_write(&sch->cds, features.features);
sch->curr_status.scsw.count = ccw.count - sizeof(features);
ret = 0;
@@ -403,7 +403,7 @@ static int virtio_ccw_cb(SubchDev *sch, CCW1 ccw)
ret = -EFAULT;
} else {
ccw_dstream_read(&sch->cds, features);
-le32_to_cpus(&features.features);
+features.features = le32_to_cpu(features.features);
if (features.index == 0) {
virtio_set_features(vdev,
(vdev->guest_features &
0xULL) |
@@ -546,7 +546,7 @@ static int virtio_ccw_cb(SubchDev *sch, CCW1 ccw)
ret = -EFAULT;
} else {
ccw_dstream_read(&sch->cds, indicators);
-be64_to_cpus(&indicators);
+indicators = be64_to_cpu(indicators);
dev->indicators = get_indicator(indicators, sizeof(uint64_t));
sch->curr_status.scsw.count = ccw.count - sizeof(indicators);
ret = 0;
@@ -567,7 +567,7 @@ static int virtio_ccw_cb(SubchDev *sch, CCW1 ccw)
ret = -EFAULT;
} else {
ccw_dstream_read(&sch->cds, indicators);
-be64_to_cpus(&indicators);
+indicators = be64_to_cpu(indicators);
dev->indicators2 = get_indicator(indicators, sizeof(uint64_t));
sch->curr_status.scsw.count = ccw.count - sizeof(indicators);
ret = 0;
@@ -588,14 +588,14 @@ static int virtio_ccw_cb(SubchDev *sch, CCW1 ccw)
ret = -EFAULT;
} else {
ccw_dstream_read(&sch->cds, vq_config.index);
-be16_to_cpus(&vq_config.index);
+vq_config.index = be16_to_cpu(vq_config.index);
if (vq_config.index >= VIRTIO_QUEUE_MAX) {
ret = -EINVAL;
break;
}
vq_config.num_max = virtio_queue_get_num(vdev,
vq_config.index);
-cpu_to_be16s(&vq_config.num_max);
+vq_config.num_max = cpu_to_be16(vq_config.num_max);
ccw_dstream_write(&sch->cds, vq_config.num_max);
sch->curr_status.scsw.count = ccw.count - sizeof(vq_config);
ret = 0;
@@ -621,9 +621,11 @@ static int virtio_ccw_cb(SubchDev *sch, CCW1 ccw)
if (ccw_dstream_read(&sch->cds, thinint)) {
ret = -EFAULT;
} else {
-be64_to_cpus(&thinint.ind_bit);
-be64_to_cpus(&thinint.summary_indicator);
-be64_to_cpus(&thinint.device_indicator);
+thinint.ind_bit = be64_to_cpu(
[Qemu-devel] [PATCH] target/i386/kvm.c: Don't mark cpuid_data as QEMU_PACKED
clang complains about taking the address of a packed
member of a struct:
target/i386/kvm.c:1245:27: warning: taking address of packed member 'cpuid' of
class or structure '' may result in an unaligned pointer value
[-Waddress-of-packed-member]
c = cpuid_find_entry(&cpuid_data.cpuid, 1, 0);
^~~~
target/i386/kvm.c:1297:31: warning: taking address of packed member 'cpuid' of
class or structure '' may result in an unaligned pointer value
[-Waddress-of-packed-member]
c = cpuid_find_entry(&cpuid_data.cpuid, kvm_base, 0);
^~~~
The kernel's definitions of struct kvm_cpuid2 and struct
kvm_cpuid_entry2 are carefully set up with padding fields
so that there is no between-struct padding anyway, so
the QEMU_PACKED annotation is unnecessary and might result
in the compiler generating worse code. Drop it, and instead
assert at build time that there is no stray padding.
Signed-off-by: Peter Maydell
---
target/i386/kvm.c | 10 +-
1 file changed, 9 insertions(+), 1 deletion(-)
diff --git a/target/i386/kvm.c b/target/i386/kvm.c
index b2401d13ea7..739cf8c8ea1 100644
--- a/target/i386/kvm.c
+++ b/target/i386/kvm.c
@@ -864,7 +864,15 @@ int kvm_arch_init_vcpu(CPUState *cs)
struct {
struct kvm_cpuid2 cpuid;
struct kvm_cpuid_entry2 entries[KVM_MAX_CPUID_ENTRIES];
-} QEMU_PACKED cpuid_data;
+} cpuid_data;
+/*
+ * The kernel defines these structs with padding fields so there
+ * should be no extra padding in our cpuid_data struct.
+ */
+QEMU_BUILD_BUG_ON(sizeof(cpuid_data) !=
+ sizeof(struct kvm_cpuid2) +
+ sizeof(struct kvm_cpuid_entry2) * KVM_MAX_CPUID_ENTRIES);
+
X86CPU *cpu = X86_CPU(cs);
CPUX86State *env = &cpu->env;
uint32_t limit, i, j, cpuid_i;
--
2.19.2
Re: [Qemu-devel] [PATCH 5/6] hw/nvram/fw_cfg: Add HMP 'info fw_cfg' command
* Philippe Mathieu-Daudé (phi...@redhat.com) wrote:
> $ qemu-system-x86_64 -S -monitor stdio
> (qemu) info fw_cfg
> TypePermSizeSpecific Order Info
>signature RO 4 QEMU
> id RO 4 0x0003
> uuid RO 16
> ----
> ram_size RO 8 0x0800
>nographic RO 2 0x
> nb_cpus RO 2 0x0001
> numa RO 16
>boot_menu RO 2 0x
> max_cpus RO 2 0x0001
> file_dir RO2052
> file (id 1) RO 36 160 etc/acpi/rsdp
> file (id 2) RO 131072 130 etc/acpi/tables
> file (id 3) RO 4 15 etc/boot-fail-wait
> file (id 4) RO 20 40 etc/e820
> file (id 5) RO 31 30 etc/smbios/smbios-anchor
> file (id 6) RO 320 20 etc/smbios/smbios-tables
> file (id 7) RO 6 90 etc/system-states
> file (id 8) RO4096 140 etc/table-loader
> file (id 10) RO9216 55 genroms/kvmvapic.bin
> uuid RO 4 (arch spec)
> 0100----
> ram_size RO 324 (arch spec)
>nographic RO 121 (arch spec)
> (qemu)
>
> Signed-off-by: Philippe Mathieu-Daudé
> ---
> hmp-commands-info.hx | 14 +
> hw/nvram/fw_cfg.c | 115 ++
> include/hw/nvram/fw_cfg.h | 2 +
> 3 files changed, 131 insertions(+)
>
> diff --git a/hmp-commands-info.hx b/hmp-commands-info.hx
> index cbee8b944d..9e86dceeb4 100644
> --- a/hmp-commands-info.hx
> +++ b/hmp-commands-info.hx
> @@ -916,6 +916,20 @@ STEXI
> @item info sev
> @findex info sev
> Show SEV information.
> +ETEXI
> +
> +{
> +.name = "fw_cfg",
> +.args_type = "",
> +.params = "",
> +.help = "Display the table of the fw_cfg entries registered",
> +.cmd= hmp_info_fw_cfg,
> +},
> +
> +STEXI
> +@item info fw_cfg
> +@findex info fw_cfg
> +Show roms.
> ETEXI
>
> STEXI
> diff --git a/hw/nvram/fw_cfg.c b/hw/nvram/fw_cfg.c
> index 582653f07e..50525ec1dd 100644
> --- a/hw/nvram/fw_cfg.c
> +++ b/hw/nvram/fw_cfg.c
> @@ -36,6 +36,7 @@
> #include "qemu/config-file.h"
> #include "qemu/cutils.h"
> #include "qapi/error.h"
> +#include "monitor/monitor.h"
>
> #define FW_CFG_FILE_SLOTS_DFLT 0x20
>
> @@ -1164,3 +1165,117 @@ static void fw_cfg_register_types(void)
> }
>
> type_init(fw_cfg_register_types)
> +
> +static const char *fw_cfg_wellknown_entries[0x20] = {
That magic 0x20 is a shame; we should probably have a #define
for that max, or perhaps it can just be removed.
> +[FW_CFG_SIGNATURE] = "signature",
> +[FW_CFG_ID] = "id",
> +[FW_CFG_UUID] = "uuid",
> +[FW_CFG_RAM_SIZE] = "ram_size",
> +[FW_CFG_NOGRAPHIC] = "nographic",
> +[FW_CFG_NB_CPUS] = "nb_cpus",
> +[FW_CFG_MACHINE_ID] = "machine_id",
> +[FW_CFG_KERNEL_ADDR] = "kernel_addr",
> +[FW_CFG_KERNEL_SIZE] = "kernel_size",
> +[FW_CFG_KERNEL_CMDLINE] = "kernel_cmdline",
> +[FW_CFG_INITRD_ADDR] = "initrd_addr",
> +[FW_CFG_INITRD_SIZE] = "initdr_size",
> +[FW_CFG_BOOT_DEVICE] = "boot_device",
> +[FW_CFG_NUMA] = "numa",
> +[FW_CFG_BOOT_MENU] = "boot_menu",
> +[FW_CFG_MAX_CPUS] = "max_cpus",
> +[FW_CFG_KERNEL_ENTRY] = "kernel_entry",
> +[FW_CFG_KERNEL_DATA] = "kernel_data",
> +[FW_CFG_INITRD_DATA] = "initrd_data",
> +[FW_CFG_CMDLINE_ADDR] = "cmdline_addr",
> +[FW_CFG_CMDLINE_SIZE] = "cmdline_size",
> +[FW_CFG_CMDLINE_DATA] = "cmdline_data",
> +[FW_CFG_SETUP_ADDR] = "setup_addr",
> +[FW_CFG_SETUP_SIZE] = "setup_size",
> +[FW_CFG_SETUP_DATA] = "setup_data",
> +[FW_CFG_FILE_DIR] = "file_dir",
> +};
> +
> +void hmp_info_fw_cfg(Monitor *mon, const QDict *qdict)
> +{
> +FWCfgState *s = fw_cfg_find();
> +int arch, key;
> +
> +monitor_printf(mon, "%12s %5s %7s %9s %6s %-24s\n",
> + "Type", "Perm", "Size", "Specific", "Order", "Info");
> +for (arch = 0; arch < ARRAY_SIZE(s->entries); ++arch) {
> +for (key = 0; key < fw_cfg_max_entry(s); ++key) {
> +FWCfgEntry *e = &s->entries[arch][key];
> +const char *perm = e->allow_write ? "RW" : "RO";
> +const char *arch_spec = arch ? "arch_spec" : "";
> +char *type, *info = NULL;
> +
> +if (!e->len) {
> +continue;
> +}
> +if (key >= FW_CFG_FILE_FIRST) {
> +int id = key - FW_CFG_FILE_FIRST;
> +const char *path = s->files->f[id].name;
> +
> +
Re: [Qemu-devel] [PATCH 3/3] uuid: Make qemu_uuid_bswap() take and return a QemuUUID
On Mon, Dec 10, 2018 at 3:27 PM Peter Maydell wrote: > > Currently qemu_uuid_bswap() takes a pointer to the QemuUUID to > be byte-swapped. This means it can't be used when the UUID > to be swapped is in a packed member of a struct. It's also > out of line with the general bswap*() functions we provide > in bswap.h, which take the value to be swapped and return it. > > Make qemu_uuid_bswap() take a QemuUUID and return the swapped version. > > This fixes some clang warnings about taking the address of > a packed struct member in block/vdi.c. > > Signed-off-by: Peter Maydell Reviewed-by: Marc-André Lureau > --- > include/qemu/uuid.h | 2 +- > block/vdi.c | 16 > hw/acpi/vmgenid.c| 6 ++ > tests/vmgenid-test.c | 2 +- > util/uuid.c | 10 +- > 5 files changed, 17 insertions(+), 19 deletions(-) > > diff --git a/include/qemu/uuid.h b/include/qemu/uuid.h > index 09489ce5c5e..037357d990b 100644 > --- a/include/qemu/uuid.h > +++ b/include/qemu/uuid.h > @@ -56,6 +56,6 @@ char *qemu_uuid_unparse_strdup(const QemuUUID *uuid); > > int qemu_uuid_parse(const char *str, QemuUUID *uuid); > > -void qemu_uuid_bswap(QemuUUID *uuid); > +QemuUUID qemu_uuid_bswap(QemuUUID uuid); > > #endif > diff --git a/block/vdi.c b/block/vdi.c > index 4cc726047c3..0c34f6bae46 100644 > --- a/block/vdi.c > +++ b/block/vdi.c > @@ -203,10 +203,10 @@ static void vdi_header_to_cpu(VdiHeader *header) > header->block_extra = le32_to_cpu(header->block_extra); > header->blocks_in_image = le32_to_cpu(header->blocks_in_image); > header->blocks_allocated = le32_to_cpu(header->blocks_allocated); > -qemu_uuid_bswap(&header->uuid_image); > -qemu_uuid_bswap(&header->uuid_last_snap); > -qemu_uuid_bswap(&header->uuid_link); > -qemu_uuid_bswap(&header->uuid_parent); > +header->uuid_image = qemu_uuid_bswap(header->uuid_image); > +header->uuid_last_snap = qemu_uuid_bswap(header->uuid_last_snap); > +header->uuid_link = qemu_uuid_bswap(header->uuid_link); > +header->uuid_parent = qemu_uuid_bswap(header->uuid_parent); > } > > static void vdi_header_to_le(VdiHeader *header) > @@ -227,10 +227,10 @@ static void vdi_header_to_le(VdiHeader *header) > header->block_extra = cpu_to_le32(header->block_extra); > header->blocks_in_image = cpu_to_le32(header->blocks_in_image); > header->blocks_allocated = cpu_to_le32(header->blocks_allocated); > -qemu_uuid_bswap(&header->uuid_image); > -qemu_uuid_bswap(&header->uuid_last_snap); > -qemu_uuid_bswap(&header->uuid_link); > -qemu_uuid_bswap(&header->uuid_parent); > +header->uuid_image = qemu_uuid_bswap(header->uuid_image); > +header->uuid_last_snap = qemu_uuid_bswap(header->uuid_last_snap); > +header->uuid_link = qemu_uuid_bswap(header->uuid_link); > +header->uuid_parent = qemu_uuid_bswap(header->uuid_parent); > } > > static void vdi_header_print(VdiHeader *header) > diff --git a/hw/acpi/vmgenid.c b/hw/acpi/vmgenid.c > index d78b579a201..02717a8b0dc 100644 > --- a/hw/acpi/vmgenid.c > +++ b/hw/acpi/vmgenid.c > @@ -30,8 +30,7 @@ void vmgenid_build_acpi(VmGenIdState *vms, GArray > *table_data, GArray *guid, > * first, since that's what the guest expects > */ > g_array_set_size(guid, VMGENID_FW_CFG_SIZE - ARRAY_SIZE(guid_le.data)); > -guid_le = vms->guid; > -qemu_uuid_bswap(&guid_le); > +guid_le = qemu_uuid_bswap(vms->guid); > /* The GUID is written at a fixed offset into the fw_cfg file > * in order to implement the "OVMF SDT Header probe suppressor" > * see docs/specs/vmgenid.txt for more details > @@ -149,8 +148,7 @@ static void vmgenid_update_guest(VmGenIdState *vms) > * however, will expect the fields to be little-endian. > * Perform a byte swap immediately before writing. > */ > -guid_le = vms->guid; > -qemu_uuid_bswap(&guid_le); > +guid_le = qemu_uuid_bswap(vms->guid); > /* The GUID is written at a fixed offset into the fw_cfg file > * in order to implement the "OVMF SDT Header probe suppressor" > * see docs/specs/vmgenid.txt for more details. > diff --git a/tests/vmgenid-test.c b/tests/vmgenid-test.c > index 0a6fb55f2eb..98db43f5a65 100644 > --- a/tests/vmgenid-test.c > +++ b/tests/vmgenid-test.c > @@ -110,7 +110,7 @@ static void read_guid_from_memory(QemuUUID *guid) > /* The GUID is in little-endian format in the guest, while QEMU > * uses big-endian. Swap after reading. > */ > -qemu_uuid_bswap(guid); > +*guid = qemu_uuid_bswap(*guid); > } > > static void read_guid_from_monitor(QemuUUID *guid) > diff --git a/util/uuid.c b/util/uuid.c > index ebf06c049ad..5787f0978c1 100644 > --- a/util/uuid.c > +++ b/util/uuid.c > @@ -110,10 +110,10 @@ int qemu_uuid_parse(const char *str, QemuUUID *uuid) > > /* Swap from UUID format endian (BE) to the opposite or vice versa. > */ > -void qemu_uu
Re: [Qemu-devel] [RFC v2 06/38] plugin: add core code
> From: Emilio G. Cota [mailto:c...@braap.org]
> +/*
> + * A dynamic callback has an insertion point that is determined at run-time.
> + * Usually the insertion point is somewhere in the code cache; think for
> + * instance of a callback to be called upon the execution of a particular TB.
> + */
> +struct qemu_plugin_dyn_cb {
> +union qemu_plugin_cb_sig f;
> +void *userp;
> +unsigned tcg_flags;
> +enum plugin_dyn_cb_type type;
> +/* @rw applies to mem callbacks only (both regular and inline) */
> +enum qemu_plugin_mem_rw rw;
> +/* fields specific to each dyn_cb type go here */
> +union {
> +struct {
> +bool haddr;
> +} mem;
> +struct {
> +enum qemu_plugin_op op;
> +uint64_t imm;
> +} inline_insn;
> +};
> +};
> +
> +struct qemu_plugin_dyn_cb_arr {
> +struct qemu_plugin_dyn_cb *data;
> +size_t n;
> +size_t capacity;
> +};
> +
Why not list or something dynamic? Is the indexing required?
Can you add the comments for the data structures and functions?
It is very hard to seek through the whole patch to get the details about them.
Pavel Dovgalyuk
[Qemu-devel] [PATCH 1/3] block/vpc: Don't take address of fields in packed structs
Taking the address of a field in a packed struct is a bad idea, because it might not be actually aligned enough for that pointer type (and thus cause a crash on dereference on some host architectures). Newer versions of clang warn about this. Avoid the bug by generating the UUID into a local variable which is definitely safely aligned and then copying it into place. Signed-off-by: Peter Maydell --- block/vpc.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/block/vpc.c b/block/vpc.c index 80c5b2b197e..968d80ae461 100644 --- a/block/vpc.c +++ b/block/vpc.c @@ -979,6 +979,7 @@ static int coroutine_fn vpc_co_create(BlockdevCreateOptions *opts, int64_t total_size; int disk_type; int ret = -EIO; +QemuUUID uuid; assert(opts->driver == BLOCKDEV_DRIVER_VPC); vpc_opts = &opts->u.vpc; @@ -1062,7 +1063,8 @@ static int coroutine_fn vpc_co_create(BlockdevCreateOptions *opts, footer->type = cpu_to_be32(disk_type); -qemu_uuid_generate(&footer->uuid); +qemu_uuid_generate(&uuid); +footer->uuid = uuid; footer->checksum = cpu_to_be32(vpc_checksum(buf, HEADER_SIZE)); -- 2.19.2
[Qemu-devel] [PATCH 3/3] uuid: Make qemu_uuid_bswap() take and return a QemuUUID
Currently qemu_uuid_bswap() takes a pointer to the QemuUUID to
be byte-swapped. This means it can't be used when the UUID
to be swapped is in a packed member of a struct. It's also
out of line with the general bswap*() functions we provide
in bswap.h, which take the value to be swapped and return it.
Make qemu_uuid_bswap() take a QemuUUID and return the swapped version.
This fixes some clang warnings about taking the address of
a packed struct member in block/vdi.c.
Signed-off-by: Peter Maydell
---
include/qemu/uuid.h | 2 +-
block/vdi.c | 16
hw/acpi/vmgenid.c| 6 ++
tests/vmgenid-test.c | 2 +-
util/uuid.c | 10 +-
5 files changed, 17 insertions(+), 19 deletions(-)
diff --git a/include/qemu/uuid.h b/include/qemu/uuid.h
index 09489ce5c5e..037357d990b 100644
--- a/include/qemu/uuid.h
+++ b/include/qemu/uuid.h
@@ -56,6 +56,6 @@ char *qemu_uuid_unparse_strdup(const QemuUUID *uuid);
int qemu_uuid_parse(const char *str, QemuUUID *uuid);
-void qemu_uuid_bswap(QemuUUID *uuid);
+QemuUUID qemu_uuid_bswap(QemuUUID uuid);
#endif
diff --git a/block/vdi.c b/block/vdi.c
index 4cc726047c3..0c34f6bae46 100644
--- a/block/vdi.c
+++ b/block/vdi.c
@@ -203,10 +203,10 @@ static void vdi_header_to_cpu(VdiHeader *header)
header->block_extra = le32_to_cpu(header->block_extra);
header->blocks_in_image = le32_to_cpu(header->blocks_in_image);
header->blocks_allocated = le32_to_cpu(header->blocks_allocated);
-qemu_uuid_bswap(&header->uuid_image);
-qemu_uuid_bswap(&header->uuid_last_snap);
-qemu_uuid_bswap(&header->uuid_link);
-qemu_uuid_bswap(&header->uuid_parent);
+header->uuid_image = qemu_uuid_bswap(header->uuid_image);
+header->uuid_last_snap = qemu_uuid_bswap(header->uuid_last_snap);
+header->uuid_link = qemu_uuid_bswap(header->uuid_link);
+header->uuid_parent = qemu_uuid_bswap(header->uuid_parent);
}
static void vdi_header_to_le(VdiHeader *header)
@@ -227,10 +227,10 @@ static void vdi_header_to_le(VdiHeader *header)
header->block_extra = cpu_to_le32(header->block_extra);
header->blocks_in_image = cpu_to_le32(header->blocks_in_image);
header->blocks_allocated = cpu_to_le32(header->blocks_allocated);
-qemu_uuid_bswap(&header->uuid_image);
-qemu_uuid_bswap(&header->uuid_last_snap);
-qemu_uuid_bswap(&header->uuid_link);
-qemu_uuid_bswap(&header->uuid_parent);
+header->uuid_image = qemu_uuid_bswap(header->uuid_image);
+header->uuid_last_snap = qemu_uuid_bswap(header->uuid_last_snap);
+header->uuid_link = qemu_uuid_bswap(header->uuid_link);
+header->uuid_parent = qemu_uuid_bswap(header->uuid_parent);
}
static void vdi_header_print(VdiHeader *header)
diff --git a/hw/acpi/vmgenid.c b/hw/acpi/vmgenid.c
index d78b579a201..02717a8b0dc 100644
--- a/hw/acpi/vmgenid.c
+++ b/hw/acpi/vmgenid.c
@@ -30,8 +30,7 @@ void vmgenid_build_acpi(VmGenIdState *vms, GArray
*table_data, GArray *guid,
* first, since that's what the guest expects
*/
g_array_set_size(guid, VMGENID_FW_CFG_SIZE - ARRAY_SIZE(guid_le.data));
-guid_le = vms->guid;
-qemu_uuid_bswap(&guid_le);
+guid_le = qemu_uuid_bswap(vms->guid);
/* The GUID is written at a fixed offset into the fw_cfg file
* in order to implement the "OVMF SDT Header probe suppressor"
* see docs/specs/vmgenid.txt for more details
@@ -149,8 +148,7 @@ static void vmgenid_update_guest(VmGenIdState *vms)
* however, will expect the fields to be little-endian.
* Perform a byte swap immediately before writing.
*/
-guid_le = vms->guid;
-qemu_uuid_bswap(&guid_le);
+guid_le = qemu_uuid_bswap(vms->guid);
/* The GUID is written at a fixed offset into the fw_cfg file
* in order to implement the "OVMF SDT Header probe suppressor"
* see docs/specs/vmgenid.txt for more details.
diff --git a/tests/vmgenid-test.c b/tests/vmgenid-test.c
index 0a6fb55f2eb..98db43f5a65 100644
--- a/tests/vmgenid-test.c
+++ b/tests/vmgenid-test.c
@@ -110,7 +110,7 @@ static void read_guid_from_memory(QemuUUID *guid)
/* The GUID is in little-endian format in the guest, while QEMU
* uses big-endian. Swap after reading.
*/
-qemu_uuid_bswap(guid);
+*guid = qemu_uuid_bswap(*guid);
}
static void read_guid_from_monitor(QemuUUID *guid)
diff --git a/util/uuid.c b/util/uuid.c
index ebf06c049ad..5787f0978c1 100644
--- a/util/uuid.c
+++ b/util/uuid.c
@@ -110,10 +110,10 @@ int qemu_uuid_parse(const char *str, QemuUUID *uuid)
/* Swap from UUID format endian (BE) to the opposite or vice versa.
*/
-void qemu_uuid_bswap(QemuUUID *uuid)
+QemuUUID qemu_uuid_bswap(QemuUUID uuid)
{
-assert(QEMU_PTR_IS_ALIGNED(uuid, sizeof(uint32_t)));
-bswap32s(&uuid->fields.time_low);
-bswap16s(&uuid->fields.time_mid);
-bswap16s(&uuid->fields.time_high_and_version);
+bswap32s(&uuid.fields.time_low);
+
[Qemu-devel] [PATCH 2/3] block/vdi: Don't take address of fields in packed structs
Taking the address of a field in a packed struct is a bad idea, because
it might not be actually aligned enough for that pointer type (and
thus cause a crash on dereference on some host architectures). Newer
versions of clang warn about this.
Instead of passing UUID related functions the address of a possibly
unaligned QemuUUID struct, use local variables and then copy to/from
the struct field as appropriate.
Signed-off-by: Peter Maydell
---
block/vdi.c | 38 +-
1 file changed, 25 insertions(+), 13 deletions(-)
diff --git a/block/vdi.c b/block/vdi.c
index 2380daa583e..4cc726047c3 100644
--- a/block/vdi.c
+++ b/block/vdi.c
@@ -235,7 +235,8 @@ static void vdi_header_to_le(VdiHeader *header)
static void vdi_header_print(VdiHeader *header)
{
-char uuid[37];
+char uuidstr[37];
+QemuUUID uuid;
logout("text%s", header->text);
logout("signature 0x%08x\n", header->signature);
logout("header size 0x%04x\n", header->header_size);
@@ -254,14 +255,18 @@ static void vdi_header_print(VdiHeader *header)
logout("block extra 0x%04x\n", header->block_extra);
logout("blocks tot. 0x%04x\n", header->blocks_in_image);
logout("blocks all. 0x%04x\n", header->blocks_allocated);
-qemu_uuid_unparse(&header->uuid_image, uuid);
-logout("uuid image %s\n", uuid);
-qemu_uuid_unparse(&header->uuid_last_snap, uuid);
-logout("uuid snap %s\n", uuid);
-qemu_uuid_unparse(&header->uuid_link, uuid);
-logout("uuid link %s\n", uuid);
-qemu_uuid_unparse(&header->uuid_parent, uuid);
-logout("uuid parent %s\n", uuid);
+uuid = header->uuid_image;
+qemu_uuid_unparse(&uuid, uuidstr);
+logout("uuid image %s\n", uuidstr);
+uuid = header->uuid_last_snap;
+qemu_uuid_unparse(&uuid, uuidstr);
+logout("uuid snap %s\n", uuidstr);
+uuid = header->uuid_link;
+qemu_uuid_unparse(&uuid, uuidstr);
+logout("uuid link %s\n", uuidstr);
+uuid = header->uuid_parent;
+qemu_uuid_unparse(&uuid, uuidstr);
+logout("uuid parent %s\n", uuidstr);
}
static int coroutine_fn vdi_co_check(BlockDriverState *bs, BdrvCheckResult
*res,
@@ -368,6 +373,7 @@ static int vdi_open(BlockDriverState *bs, QDict *options,
int flags,
size_t bmap_size;
int ret;
Error *local_err = NULL;
+QemuUUID uuid_link, uuid_parent;
bs->file = bdrv_open_child(NULL, options, "file", bs, &child_file,
false, errp);
@@ -395,6 +401,9 @@ static int vdi_open(BlockDriverState *bs, QDict *options,
int flags,
goto fail;
}
+uuid_link = header.uuid_link;
+uuid_parent = header.uuid_parent;
+
if (header.disk_size % SECTOR_SIZE != 0) {
/* 'VBoxManage convertfromraw' can create images with odd disk sizes.
We accept them but round the disk size to the next multiple of
@@ -444,11 +453,11 @@ static int vdi_open(BlockDriverState *bs, QDict *options,
int flags,
(uint64_t)header.blocks_in_image * header.block_size);
ret = -ENOTSUP;
goto fail;
-} else if (!qemu_uuid_is_null(&header.uuid_link)) {
+} else if (!qemu_uuid_is_null(&uuid_link)) {
error_setg(errp, "unsupported VDI image (non-NULL link UUID)");
ret = -ENOTSUP;
goto fail;
-} else if (!qemu_uuid_is_null(&header.uuid_parent)) {
+} else if (!qemu_uuid_is_null(&uuid_parent)) {
error_setg(errp, "unsupported VDI image (non-NULL parent UUID)");
ret = -ENOTSUP;
goto fail;
@@ -733,6 +742,7 @@ static int coroutine_fn
vdi_co_do_create(BlockdevCreateOptions *create_options,
BlockDriverState *bs_file = NULL;
BlockBackend *blk = NULL;
uint32_t *bmap = NULL;
+QemuUUID uuid;
assert(create_options->driver == BLOCKDEV_DRIVER_VDI);
vdi_opts = &create_options->u.vdi;
@@ -819,8 +829,10 @@ static int coroutine_fn
vdi_co_do_create(BlockdevCreateOptions *create_options,
if (image_type == VDI_TYPE_STATIC) {
header.blocks_allocated = blocks;
}
-qemu_uuid_generate(&header.uuid_image);
-qemu_uuid_generate(&header.uuid_last_snap);
+qemu_uuid_generate(&uuid);
+header.uuid_image = uuid;
+qemu_uuid_generate(&uuid);
+header.uuid_last_snap = uuid;
/* There is no need to set header.uuid_link or header.uuid_parent here. */
if (VDI_DEBUG) {
vdi_header_print(&header);
--
2.19.2
[Qemu-devel] [PATCH 0/3] block: fix last address-of-packed-member warnings
This patchset fixes the remaining clang warnings in the block/ code about taking the address of a packed struct member, which are all in block/vpc and block/vdi code handling UUIDs. Mostly I fix these by copying the unaligned field to/from a local variable. In the case of qemu_uuid_bswap() I opted to change the API to take and return the QemuUUID rather than taking a pointer to it, which makes almost all the callsites simpler. This does mean a struct copy but the struct is only 16 bytes and I didn't judge any of the callsites performance-sensitive enough to care about a struct copy of that size. As usual, tested with "make check" only. thanks -- PMM Peter Maydell (3): block/vpc: Don't take address of fields in packed structs block/vdi: Don't take address of fields in packed structs uuid: Make qemu_uuid_bswap() take and return a QemuUUID include/qemu/uuid.h | 2 +- block/vdi.c | 54 +++- block/vpc.c | 4 +++- hw/acpi/vmgenid.c| 6 ++--- tests/vmgenid-test.c | 2 +- util/uuid.c | 10 6 files changed, 45 insertions(+), 33 deletions(-) -- 2.19.2
Re: [Qemu-devel] [PATCH] cpus.c: Fix race condition in cpu_stop_current()
On Mon, 10 Dec 2018 at 11:06, Alex Bennée wrote:
>
>
> Peter Maydell writes:
> > We discussed this a little while back:
> > https://lists.gnu.org/archive/html/qemu-devel/2018-10/msg00154.html
> > and Jaap reported a bug which I suspect of being the same thing:
> > https://lists.gnu.org/archive/html/qemu-discuss/2018-10/msg00014.html
> >
> > Annoyingly I have lost the test case that demonstrated this
> > race, but I analysed it at the time and this should definitely
> > fix it. I have opted not to try to address any of the other
> > possible cleanup here (eg vm_stop() has a potential similar
> > race if called from a vcpu thread I suspect), since it gets
> > pretty tangled.
> >
> > Jaap: could you test whether this patch fixes the issue you
> > were seeing, please?
> > ---
> > cpus.c | 3 ++-
> > 1 file changed, 2 insertions(+), 1 deletion(-)
> >
> > diff --git a/cpus.c b/cpus.c
> > index 0ddeeefc14f..b09b7027126 100644
> > --- a/cpus.c
> > +++ b/cpus.c
> > @@ -2100,7 +2100,8 @@ void qemu_init_vcpu(CPUState *cpu)
> > void cpu_stop_current(void)
> > {
> > if (current_cpu) {
> > -qemu_cpu_stop(current_cpu, true);
> > +current_cpu->stop = true;
> > +cpu_exit(current_cpu);
>
> Should the FIXME in vm_stop also be fixed?
>
> /*
> * FIXME: should not return to device code in case
> * vm_stop() has been requested.
> */
> cpu_stop_current();
> return 0;
This is one of the things I had in mind with:
> > I have opted not to try to address any of the other
> > possible cleanup here (eg vm_stop() has a potential similar
> > race if called from a vcpu thread I suspect), since it gets
> > pretty tangled.
though I might actually have meant pause_all_vcpus().
(For pause_all_vcpus() I think the correct thing is to
fix the hw/i386/kvmvapic.c code to work in some other way,
and then assert that pause_all_vcpus() is never called from
the VCPU thread.)
At any rate, this code is quite complicated, so I think
it's worth just fixing this specific race without getting
tangled up in everything else we could potentially refactor.
I'm not even sure how we would arrange for vm_stop() to
avoid returning to device emulation code if it has been
called from there -- I would favour instead changing/defining
the semantics to be like the shutdown-request and reset-request
where the device code expects that control will return but
the VM stop happens at the next opportunity, ie as soon
as the execution of the insn which wrote to the device
register has completed.
thanks
-- PMM
Re: [Qemu-devel] [PATCH v6 21/27] qapi: add #if conditions to generated code members
On Mon, Dec 10, 2018 at 2:11 PM Markus Armbruster wrote: > > Markus Armbruster writes: > > > Marc-André Lureau writes: > > > >> Hi > >> On Thu, Dec 6, 2018 at 9:42 PM Markus Armbruster wrote: > >>> > >>> Marc-André Lureau writes: > >>> > >>> > Wrap generated enum/struct members and code with #if/#endif, using the > >>> > >>> enum and struct members > >> > >> ok > >> > >>> > >>> > .ifcond members added in the previous patches. > >>> > > >>> > Some types generate both enum and struct members for example, so a > >>> > step-by-step is unnecessarily complicated to deal with (it would > >>> > easily generate invalid intermediary code). > >>> > >>> Can you give an example of a schema definition that would lead to > >>> complications? > >>> > >> > >> Honestly, I don't remember well (it's been a while I wrote that code). > > > > I know... > > > >> It must be related to implicit enums, such as union kind... If there > >> is no strong need to split this patch, I would rather not do that > >> extra work. > > > > I'm not looking for reasons to split this patch, I'm looking for > > stronger reasons to keep it just like it is :) > > > > Your hunch that complications would arise for simple unions plausible: > > there the same conditional needs to be applied both to the C enum's > > member and the C union member. > > > > For the generated C code to compile, each union tag enum member > > conditional must imply the corresponding variant conditional. > > > > For flat unions, the two are separate. The QAPI generator makes no > > effort to check the enum member's if condition implies the union > > variant's if condition; if you mess them up in the schema, you get to > > deal with the C compilation errors. > > > > For simple unions, the two are one. > > > > If we separate the generator updates for enums and for union members, > > and do enum members first, then unions with conditional tag members > > can't compile. Corrollary: simple unions with conditional variants > > can't compile. > > > > What if we do union members first? > > > > Again, I'm not asking for patch splitting here, I'm just trying to > > arrive at a clearer understanding to avoid making insufficiently > > supported claims in the commit message. The combined patch looks small > > and clean enough to keep it combined. > > > > [...] > > What about this commit message: > > qapi: Add #if conditions to generated code members > > Wrap generated enum and struct members and their supporting code with > #if/#endif, using the .ifcond members added in the previous patches. > > We do enum and struct in a single patch because union tag enum and the > associated variants tie them together, and dealing with that to split > the patch doesn't seem worthwhile. > ack, thanks -- Marc-André Lureau
Re: [Qemu-devel] [PATCH v6 15/27] qapi: rename allow_dict to allow_implicit
Hi
On Mon, Dec 10, 2018 at 12:51 PM Markus Armbruster wrote:
>
> Marc-André Lureau writes:
>
> > On Wed, Dec 5, 2018 at 10:41 PM Markus Armbruster wrote:
> >>
> >> Marc-André Lureau writes:
> >>
> >> > This makes it a bit clearer what is the intent of the dictionnary for
> >>
> >> dictionary
> >
> > sigh, this must be a very common misspell (dictionnaire in french)
>
> Muscle memory...
>
> >> > the check_type() function, since there was some confusion on a
> >> > previous iteration of this series.
> >>
> >> I don't remember... got a pointer to that confusion handy?
> >
> > https://lists.nongnu.org/archive/html/qemu-devel/2017-12/msg01584.html
>
> Thanks!
>
> My review comment was
>
> > diff --git a/scripts/qapi.py b/scripts/qapi.py
> > index df2a304e8f..15711f96fa 100644
> > --- a/scripts/qapi.py
> > +++ b/scripts/qapi.py
> > @@ -696,7 +696,15 @@ def check_type(info, source, value,
> allow_array=False,
> > return
> >
> > if not allow_dict:
> > -raise QAPISemError(info, "%s should be a type name" % source)
> > +if isinstance(value, dict) and 'type' in value:
> > +check_type(info, source, value['type'], allow_array,
> > + allow_dict, allow_optional, allow_metas)
> > +if 'if' in value:
> > +check_if(value, info)
> > +check_unknown_keys(info, value, {'type', 'if'})
> > +return
> > +else:
> > +raise QAPISemError(info, "%s should be a type name" %
> source)
>
> @allow_dict becomes a misnomer: dictionaries are now always allowed, but
> they have different meaning (implicit type vs. named type with
> additional attributes).
>
> Rename to @allow_implicit?
>
> As far as I can tell, it's not an issue in the current version of your
> patches anymore:
>
> def check_type(info, source, value, allow_array=False,
>allow_implicit=False, allow_optional=False,
>allow_metas=[]):
> global all_names
>
> if value is None:
> return
>
> # Check if array type for value is okay
> if isinstance(value, list):
> if not allow_array:
> raise QAPISemError(info, "%s cannot be an array" % source)
> if len(value) != 1 or not isinstance(value[0], str):
> raise QAPISemError(info,
>"%s: array type must contain single type
> name" %
>source)
> value = value[0]
>
> # Check if type name for value is okay
> if isinstance(value, str):
> if value not in all_names:
> raise QAPISemError(info, "%s uses unknown type '%s'"
>% (source, value))
> if not all_names[value] in allow_metas:
> raise QAPISemError(info, "%s cannot use %s type '%s'" %
>(source, all_names[value], value))
> return
>
> if not allow_implicit:
> raise QAPISemError(info, "%s should be a type name" % source)
>
> if not isinstance(value, OrderedDict):
> raise QAPISemError(info,
>"%s should be a dictionary or type name" %
> source)
>
> # value is a dictionary, check that each member is okay
> for (key, arg) in value.items():
> check_name(info, "Member of %s" % source, key,
>allow_optional=allow_optional)
> if c_name(key, False) == 'u' or c_name(key,
> False).startswith('has_'):
> raise QAPISemError(info, "Member of %s uses reserved name
> '%s'"
>% (source, key))
> # Todo: allow dictionaries to represent default values of
> # an optional argument.
> if isinstance(arg, dict):
> check_known_keys(info, "member '%s' of %s" % (key, source),
> arg, ['type'], ['if'])
> typ = arg['type']
> else:
> typ = arg
> check_type(info, "Member '%s' of %s" % (key, source),
>typ, allow_array=True,
>allow_metas=['built-in', 'union', 'alternate',
> 'struct',
> 'enum'])
>
>
> >> > Suggested-by: Markus Armbruster
> >> > Signed-off-by: Marc-André Lureau
>
Feel free to drop it (allow_implicit seems a bit more clear to me though).
--
Marc-André Lureau
Re: [Qemu-devel] [PATCH for-4.0 v7 01/27] qapi: make sure osdep.h is included in type headers
Hi
On Mon, Dec 10, 2018 at 1:52 PM Markus Armbruster wrote:
>
> Marc-André Lureau writes:
>
> > Now that the schema can be configured, it is crucial that all types
> > are configured the same. Make sure config-host.h is included, by
> > checking osdep.h inclusion. The build-sys tracks the dependency and
> > rebuilds the types if the configuration changed.
> >
> > Signed-off-by: Marc-André Lureau
> > ---
> > scripts/qapi/types.py | 3 +++
> > 1 file changed, 3 insertions(+)
> >
> > diff --git a/scripts/qapi/types.py b/scripts/qapi/types.py
> > index fd7808103c..3bb9cb6d47 100644
> > --- a/scripts/qapi/types.py
> > +++ b/scripts/qapi/types.py
> > @@ -201,6 +201,9 @@ class
> > QAPISchemaGenTypeVisitor(QAPISchemaModularCVisitor):
> > ''',
> >types=types, visit=visit))
> > self._genh.preamble_add(mcgen('''
> > +#ifndef QEMU_OSDEP_H
> > +#error Please include osdep.h first!
> > +#endif
> > #include "qapi/qapi-builtin-types.h"
> > '''))
>
> I understand why you propose this patch. The QAPI-generated headers use
> #ifdef CONFIG_FOO. The configuration header "qemu/osdep.h" must be
> consistently included before the generated headers, or else you end up
> with nasty bugs, such as the same enum having different values in
> different .o, or the same struct having a different layout.
>
> But this applies to *all* headers that use #ifdef. Why check it here,
> but not there? What makes the QAPI-generated headers special?
>
It's the discussion about #if in headers (and enums in particular)
that started this. We want to make sure all compilation units share
the same data structure/ABI. I proposed to include osdep.h in qapi
headers, but that was rejected.
The warning is a different approach. I agree it could apply to all
headers. Do you think I should update all headers as well?
--
Marc-André Lureau
Re: [Qemu-devel] [Qemu-arm] more serial ports on arm?
On 12/10/18 12:09 PM, Peter Maydell wrote: > On Mon, 10 Dec 2018 at 11:00, Philippe Mathieu-Daudé > wrote: >> >> Hi Jason, >> >> On 12/7/18 5:42 AM, Jason A. Donenfeld wrote: >>> On Tue, Nov 20, 2018 at 11:08 AM Peter Maydell >>> wrote: It's still stuck, because unconditionally adding a second serial port to the virt board breaks some commonly used existing guest code (UEFI + Linux), and it's not clear to me what the best command line UI is for allowing the user to request the second serial port. >>> >>> Can the UI be the same more or less as for x86? Specify a second >>> -serial, and then the machine gets one added? If it's in secure mode, >>> then it's added as serial 3 instead of 2, to remain backwards >>> compatible. >> >> You can use various -serial arguments. >> >> If a board supports 4 serials and you only want to see the 3rd (secure >> mode as your example) you could use: >> >> ./qemu -serial null -serial null -serial stdio > > That's the opposite of Jason's issue, which is that the 'virt' > board only supports 1 serial port but he would like 2... Oh sorry Jason, I misunderstood your question. Regards, Phil.
Re: [Qemu-devel] [PATCH for-4.0 v7 07/27] qapi: improve reporting of unknown or missing keys
On Mon, Dec 10, 2018 at 2:03 PM Markus Armbruster wrote: > > Marc-André Lureau writes: > > > Report the set of missing or unknown keys. And give a hint about the > > accepted keys. > > Suggest to add: > > The error message for multiple meta type members (visible in > tests/qapi-schema/double-type.err) is not improved. > > > Signed-off-by: Marc-André Lureau > > Reviewed-by: Markus Armbruster > ack, thanks -- Marc-André Lureau
Re: [Qemu-devel] [Qemu-arm] more serial ports on arm?
On Mon, 10 Dec 2018 at 11:00, Philippe Mathieu-Daudé wrote: > > Hi Jason, > > On 12/7/18 5:42 AM, Jason A. Donenfeld wrote: > > On Tue, Nov 20, 2018 at 11:08 AM Peter Maydell > > wrote: > >> It's still stuck, because unconditionally adding a second serial > >> port to the virt board breaks some commonly used existing guest > >> code (UEFI + Linux), and it's not clear to me what the best > >> command line UI is for allowing the user to request the second > >> serial port. > > > > Can the UI be the same more or less as for x86? Specify a second > > -serial, and then the machine gets one added? If it's in secure mode, > > then it's added as serial 3 instead of 2, to remain backwards > > compatible. > > You can use various -serial arguments. > > If a board supports 4 serials and you only want to see the 3rd (secure > mode as your example) you could use: > > ./qemu -serial null -serial null -serial stdio That's the opposite of Jason's issue, which is that the 'virt' board only supports 1 serial port but he would like 2... thanks -- PMM
Re: [Qemu-devel] [PATCH] cpus.c: Fix race condition in cpu_stop_current()
Peter Maydell writes:
> We use cpu_stop_current() to ensure the current CPU has stopped
> from places like qemu_system_reset_request(). Unfortunately its
> current implementation has a race. It calls qemu_cpu_stop(),
> which sets cpu->stopped to true even though the CPU hasn't
> actually stopped yet. The main thread will look at the flags
> set by qemu_system_reset_request() and call pause_all_vcpus().
> pause_all_vcpus() waits for every cpu to have cpu->stopped true,
> so it can continue (and we will start the system reset operation)
> before the vcpu thread has got back to its top level loop.
>
> Instead, just set cpu->stop and call cpu_exit(). This will
> cause the vcpu to exit back to the top level loop, and there
> (as part of the wait_io_event code) it will call qemu_cpu_stop().
>
> This fixes bugs where the reset request appeared to be ignored
> or the CPU misbehaved because the reset operation started
> to change vcpu state while the vcpu thread was still using it.
>
> Signed-off-by: Peter Maydell
> ---
> We discussed this a little while back:
> https://lists.gnu.org/archive/html/qemu-devel/2018-10/msg00154.html
> and Jaap reported a bug which I suspect of being the same thing:
> https://lists.gnu.org/archive/html/qemu-discuss/2018-10/msg00014.html
>
> Annoyingly I have lost the test case that demonstrated this
> race, but I analysed it at the time and this should definitely
> fix it. I have opted not to try to address any of the other
> possible cleanup here (eg vm_stop() has a potential similar
> race if called from a vcpu thread I suspect), since it gets
> pretty tangled.
>
> Jaap: could you test whether this patch fixes the issue you
> were seeing, please?
> ---
> cpus.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/cpus.c b/cpus.c
> index 0ddeeefc14f..b09b7027126 100644
> --- a/cpus.c
> +++ b/cpus.c
> @@ -2100,7 +2100,8 @@ void qemu_init_vcpu(CPUState *cpu)
> void cpu_stop_current(void)
> {
> if (current_cpu) {
> -qemu_cpu_stop(current_cpu, true);
> +current_cpu->stop = true;
> +cpu_exit(current_cpu);
Should the FIXME in vm_stop also be fixed?
/*
* FIXME: should not return to device code in case
* vm_stop() has been requested.
*/
cpu_stop_current();
return 0;
> }
> }
--
Alex Bennée
Re: [Qemu-devel] [Qemu-arm] more serial ports on arm?
Hi Jason, On 12/7/18 5:42 AM, Jason A. Donenfeld wrote: > On Tue, Nov 20, 2018 at 11:08 AM Peter Maydell > wrote: >> It's still stuck, because unconditionally adding a second serial >> port to the virt board breaks some commonly used existing guest >> code (UEFI + Linux), and it's not clear to me what the best >> command line UI is for allowing the user to request the second >> serial port. > > Can the UI be the same more or less as for x86? Specify a second > -serial, and then the machine gets one added? If it's in secure mode, > then it's added as serial 3 instead of 2, to remain backwards > compatible. You can use various -serial arguments. If a board supports 4 serials and you only want to see the 3rd (secure mode as your example) you could use: ./qemu -serial null -serial null -serial stdio serials #1 and #2 and #4 would be redirected to the null backend, thus ignored, and you bind the secure serial to your stdio.
[Qemu-devel] [Bug 1703506] Re: SMT not supported by QEMU on AMD Ryzen CPU
Error I see in terminal: AMD CPU doesn't support hyperthreading. Please configure -smp options properly. Error I see in my windows 10 vm: SYSTEM THREAD EXCEPTION NOT HANDLED I am unable to use Qemu at all. Serious problem. CPU: AMD Ryzen 5 1600X Six-Core Processor × 6 -- You received this bug notification because you are a member of qemu- devel-ml, which is subscribed to QEMU. https://bugs.launchpad.net/bugs/1703506 Title: SMT not supported by QEMU on AMD Ryzen CPU Status in QEMU: New Bug description: HyperThreading/SMT is supported by AMD Ryzen CPUs but results in this message when setting the topology to threads=2: qemu-system-x86_64: AMD CPU doesn't support hyperthreading. Please configure -smp options properly. Checking in a Windows 10 guest reveals that SMT is not enabled, and from what I understand, QEMU converts the topology from threads to cores internally on AMD CPUs. This appears to cause performance problems in the guest perhaps because programs are assuming that these threads are actual cores. Software: Linux 4.12, qemu 2.9.0 host with KVM enabled, Windows 10 pro guest To manage notifications about this bug go to: https://bugs.launchpad.net/qemu/+bug/1703506/+subscriptions
Re: [Qemu-devel] [PATCH v3 4/5] crypto/block: introduce qcrypto_block_*crypt_helper functions
On Fri 07 Dec 2018 05:13:50 PM CET, Vladimir Sementsov-Ogievskiy wrote: > Introduce QCryptoBlock-based functions and use them where possible. > This is needed to implement thread-safe encrypt/decrypt operations. > > Signed-off-by: Vladimir Sementsov-Ogievskiy > Reviewed-by: Daniel P. Berrangé Reviewed-by: Alberto Garcia Berto
Re: [Qemu-devel] [PATCH v3 3/5] crypto/block: rename qcrypto_block_*crypt_helper
On Fri 07 Dec 2018 05:13:49 PM CET, Vladimir Sementsov-Ogievskiy wrote: > Rename qcrypto_block_*crypt_helper to qcrypto_cipher_*crypt_helper, as You forgot to update the new function names in the commit message. > -int qcrypto_block_decrypt_helper(QCryptoCipher *cipher, > -int qcrypto_block_encrypt_helper(QCryptoCipher *cipher, > +int qcrypto_block_cipher_decrypt_helper(QCryptoCipher *cipher, > +int qcrypto_block_cipher_encrypt_helper(QCryptoCipher *cipher, Berto
Re: [Qemu-devel] [RFC] arm: Allow system registers for KVM guests to be changed by QEMU code
On 2018/12/6 23:14, Peter Maydell wrote:
> At the moment the Arm implementations of kvm_arch_{get,put}_registers()
> don't support having QEMU change the values of system registers
> (aka coprocessor registers for AArch32). This is because although
> kvm_arch_get_registers() calls write_list_to_cpustate() to
> update the CPU state struct fields (so QEMU code can read the
> values in the usual way), kvm_arch_put_registers() does not
> call write_cpustate_to_list(), meaning that any changes to
> the CPU state struct fields will not be passed back to KVM.
>
> The rationale for this design is documented in a comment in the
> AArch32 kvm_arch_put_registers() -- writing the values in the
> cpregs list into the CPU state struct is "lossy" because the
> write of a register might not succeed, and so if we blindly
> copy the CPU state values back again we will incorrectly
> change register values for the guest. The assumption was that
> no QEMU code would need to write to the registers.
>
> However, when we implemented debug support for KVM guests, we
> broke that assumption: the code to handle "set the guest up
> to take a breakpoint exception" does so by updating various
> guest registers including ESR_EL1.
>
> Support this by making kvm_arch_put_registers() synchronize
> CPU state back into the list. We sync only those registers
> where the initial write succeeds, which should be sufficient.
>
> Signed-off-by: Peter Maydell
Tested-by: Dongjiu Geng
> ---
> I think this patch:
> * should be necessary for the current KVM debug code to be able
>to actually set the ESR_EL1 for the guest when it wants to
>deliver a breakpoint exception to it
> * will also be necessary for Dongjiu's patchset to inject
>external aborts on memory errors
>
> I have tagged it "RFC" because I don't have a setup to test
> that; I've just given it a quick smoke test that it runs a
> VM OK. Please test this and check whether it really does
> fix the bugs I think it does :-)
I have tested this patch in my aarch64 platform,
it works well to inject external aborts on memory errors.
Thanks for this patch.
>
> Opinions welcome on whether the "try the write-and-read-back"
> approach in write_cpustate_to_list() is too hacky and it
> would be better to actually record whether write_list_to_cpustate()
> succeeded for each register. (That would require another array.)
> ---
> target/arm/cpu.h | 9 -
> target/arm/helper.c | 27 +--
> target/arm/kvm32.c | 20 ++--
> target/arm/kvm64.c | 2 ++
> target/arm/machine.c | 2 +-
> 5 files changed, 38 insertions(+), 22 deletions(-)
>
> diff --git a/target/arm/cpu.h b/target/arm/cpu.h
> index 2a73fed9a01..c0c111378ea 100644
> --- a/target/arm/cpu.h
> +++ b/target/arm/cpu.h
> @@ -2321,18 +2321,25 @@ bool write_list_to_cpustate(ARMCPU *cpu);
> /**
> * write_cpustate_to_list:
> * @cpu: ARMCPU
> + * @kvm_sync: true if this is for syncing back to KVM
> *
> * For each register listed in the ARMCPU cpreg_indexes list, write
> * its value from the ARMCPUState structure into the cpreg_values list.
> * This is used to copy info from TCG's working data structures into
> * KVM or for outbound migration.
> *
> + * @kvm_sync is true if we are doing this in order to sync the
> + * register state back to KVM. In this case we will only update
> + * values in the list if the previous list->cpustate sync actually
> + * successfully wrote the CPU state. Otherwise we will keep the value
> + * that is in the list.
> + *
> * Returns: true if all register values were read correctly,
> * false if some register was unknown or could not be read.
> * Note that we do not stop early on failure -- we will attempt
> * reading all registers in the list.
> */
> -bool write_cpustate_to_list(ARMCPU *cpu);
> +bool write_cpustate_to_list(ARMCPU *cpu, bool kvm_sync);
>
> #define ARM_CPUID_TI915T 0x54029152
> #define ARM_CPUID_TI925T 0x54029252
> diff --git a/target/arm/helper.c b/target/arm/helper.c
> index 0da1424f72d..bc2969eb4d8 100644
> --- a/target/arm/helper.c
> +++ b/target/arm/helper.c
> @@ -263,7 +263,7 @@ static bool raw_accessors_invalid(const ARMCPRegInfo *ri)
> return true;
> }
>
> -bool write_cpustate_to_list(ARMCPU *cpu)
> +bool write_cpustate_to_list(ARMCPU *cpu, bool kvm_sync)
> {
> /* Write the coprocessor state from cpu->env to the (index,value) list.
> */
> int i;
> @@ -272,6 +272,7 @@ bool write_cpustate_to_list(ARMCPU *cpu)
> for (i = 0; i < cpu->cpreg_array_len; i++) {
> uint32_t regidx = kvm_to_cpreg_id(cpu->cpreg_indexes[i]);
> const ARMCPRegInfo *ri;
> +uint64_t newval;
>
> ri = get_arm_cp_reginfo(cpu->cp_regs, regidx);
> if (!ri) {
> @@ -281,7 +282,29 @@ bool write_cpustate_to_list(ARMCPU *cpu)
> if (ri->type & ARM_CP_NO_RAW) {
> continue;
> }
> -cpu->cpreg_values[i] = read_raw_cp_reg(&cpu
Re: [Qemu-devel] [PATCH v6 21/27] qapi: add #if conditions to generated code members
Markus Armbruster writes: > Marc-André Lureau writes: > >> Hi >> On Thu, Dec 6, 2018 at 9:42 PM Markus Armbruster wrote: >>> >>> Marc-André Lureau writes: >>> >>> > Wrap generated enum/struct members and code with #if/#endif, using the >>> >>> enum and struct members >> >> ok >> >>> >>> > .ifcond members added in the previous patches. >>> > >>> > Some types generate both enum and struct members for example, so a >>> > step-by-step is unnecessarily complicated to deal with (it would >>> > easily generate invalid intermediary code). >>> >>> Can you give an example of a schema definition that would lead to >>> complications? >>> >> >> Honestly, I don't remember well (it's been a while I wrote that code). > > I know... > >> It must be related to implicit enums, such as union kind... If there >> is no strong need to split this patch, I would rather not do that >> extra work. > > I'm not looking for reasons to split this patch, I'm looking for > stronger reasons to keep it just like it is :) > > Your hunch that complications would arise for simple unions plausible: > there the same conditional needs to be applied both to the C enum's > member and the C union member. > > For the generated C code to compile, each union tag enum member > conditional must imply the corresponding variant conditional. > > For flat unions, the two are separate. The QAPI generator makes no > effort to check the enum member's if condition implies the union > variant's if condition; if you mess them up in the schema, you get to > deal with the C compilation errors. > > For simple unions, the two are one. > > If we separate the generator updates for enums and for union members, > and do enum members first, then unions with conditional tag members > can't compile. Corrollary: simple unions with conditional variants > can't compile. > > What if we do union members first? > > Again, I'm not asking for patch splitting here, I'm just trying to > arrive at a clearer understanding to avoid making insufficiently > supported claims in the commit message. The combined patch looks small > and clean enough to keep it combined. > > [...] What about this commit message: qapi: Add #if conditions to generated code members Wrap generated enum and struct members and their supporting code with #if/#endif, using the .ifcond members added in the previous patches. We do enum and struct in a single patch because union tag enum and the associated variants tie them together, and dealing with that to split the patch doesn't seem worthwhile.
Re: [Qemu-devel] [PATCH for-4.0 v7 07/27] qapi: improve reporting of unknown or missing keys
Marc-André Lureau writes: > Report the set of missing or unknown keys. And give a hint about the > accepted keys. Suggest to add: The error message for multiple meta type members (visible in tests/qapi-schema/double-type.err) is not improved. > Signed-off-by: Marc-André Lureau Reviewed-by: Markus Armbruster
Re: [Qemu-devel] [PATCH for-4.0 v7 06/27] qapi: factor out checking for keys
Marc-André Lureau writes: > Introduce a new helper function to check if the given keys are known, > and if mandatory keys are present. The function will be reused in > other places in the following code changes. > > Signed-off-by: Marc-André Lureau Reviewed-by: Markus Armbruster
Re: [Qemu-devel] [PATCH for-4.0 v7 01/27] qapi: make sure osdep.h is included in type headers
Marc-André Lureau writes:
> Now that the schema can be configured, it is crucial that all types
> are configured the same. Make sure config-host.h is included, by
> checking osdep.h inclusion. The build-sys tracks the dependency and
> rebuilds the types if the configuration changed.
>
> Signed-off-by: Marc-André Lureau
> ---
> scripts/qapi/types.py | 3 +++
> 1 file changed, 3 insertions(+)
>
> diff --git a/scripts/qapi/types.py b/scripts/qapi/types.py
> index fd7808103c..3bb9cb6d47 100644
> --- a/scripts/qapi/types.py
> +++ b/scripts/qapi/types.py
> @@ -201,6 +201,9 @@ class QAPISchemaGenTypeVisitor(QAPISchemaModularCVisitor):
> ''',
>types=types, visit=visit))
> self._genh.preamble_add(mcgen('''
> +#ifndef QEMU_OSDEP_H
> +#error Please include osdep.h first!
> +#endif
> #include "qapi/qapi-builtin-types.h"
> '''))
I understand why you propose this patch. The QAPI-generated headers use
#ifdef CONFIG_FOO. The configuration header "qemu/osdep.h" must be
consistently included before the generated headers, or else you end up
with nasty bugs, such as the same enum having different values in
different .o, or the same struct having a different layout.
But this applies to *all* headers that use #ifdef. Why check it here,
but not there? What makes the QAPI-generated headers special?
Re: [Qemu-devel] [PATCH v2 05/18] xen: add xenstore watcher infrastructure
> -Original Message-
> From: Anthony PERARD [mailto:anthony.per...@citrix.com]
> Sent: 07 December 2018 15:58
> To: Paul Durrant
> Cc: qemu-devel@nongnu.org; qemu-bl...@nongnu.org; xen-
> de...@lists.xenproject.org; Kevin Wolf ; Max Reitz
> ; Stefano Stabellini
> Subject: Re: [PATCH v2 05/18] xen: add xenstore watcher infrastructure
>
> On Thu, Dec 06, 2018 at 03:08:31PM +, Paul Durrant wrote:
> > @@ -36,6 +54,12 @@ static void xen_block_unrealize(XenDevice *xendev,
> Error **errp)
> >
> > trace_xen_block_unrealize(type, vdev->disk, vdev->partition);
> >
> > +/* Disconnect from the frontend in case this has not already
> happened */
> > +xen_block_disconnect(xendev, &local_err);
> > +if (local_err) {
> > +error_propagate(errp, local_err);
>
> If xen_block_disconnect fails, local_err is going to be reuse below. If
> it's fine to try unrealize, then `local_err=NULL` is probably enough.
>
Actually, on this path there is nothing useful to be done with an error...
disconnect really really should not fail, so I'll pass NULL instead to ignore
any error.
> > +}
> > +
> > if (blockdev_class->unrealize) {
> > blockdev_class->unrealize(blockdev, &local_err);
> > if (local_err) {
>
> [...]
>
> > +static void xen_bus_remove_watch(XenBus *xenbus, XenWatch *watch,
> > + Error **errp)
> > +{
> > +Error *local_err = NULL;
> > +
> > +trace_xen_bus_remove_watch(watch->node, watch->key, watch->token);
> > +
> > +xs_node_unwatch(xenbus->xsh, watch->node, watch->key, watch->token,
> > +&local_err);
>
> You could simply pass `errp' directly instead of having `local_err'.
>
Indeed.
Paul
> > +
> > +notifier_remove(&watch->notifier);
> > +free_watch(watch);
> > +
> > +if (local_err) {
> > +error_propagate(errp, local_err);
> > +}
> > +}
> > +
>
> --
> Anthony PERARD
Re: [Qemu-devel] [PATCH v2 03/18] xen: introduce 'xen-block', 'xen-disk' and 'xen-cdrom'
> -Original Message-
> From: Anthony PERARD [mailto:anthony.per...@citrix.com]
> Sent: 07 December 2018 15:26
> To: Paul Durrant
> Cc: qemu-devel@nongnu.org; qemu-bl...@nongnu.org; xen-
> de...@lists.xenproject.org; Kevin Wolf ; Max Reitz
> ; Stefano Stabellini
> Subject: Re: [PATCH v2 03/18] xen: introduce 'xen-block', 'xen-disk' and
> 'xen-cdrom'
>
> On Fri, Dec 07, 2018 at 02:39:40PM +, Paul Durrant wrote:
> > > -Original Message-
> > > From: Anthony PERARD [mailto:anthony.per...@citrix.com]
> > > Sent: 07 December 2018 14:35
> > > To: Paul Durrant
> > > Cc: qemu-devel@nongnu.org; qemu-bl...@nongnu.org; xen-
> > > de...@lists.xenproject.org; Kevin Wolf ; Max Reitz
> > > ; Stefano Stabellini
> > > Subject: Re: [PATCH v2 03/18] xen: introduce 'xen-block', 'xen-disk'
> and
> > > 'xen-cdrom'
> > >
> > > On Thu, Dec 06, 2018 at 03:08:29PM +, Paul Durrant wrote:
> > > > +static char *disk_to_vbd_name(unsigned int disk)
> > > > +{
> > > > +char *name, *prefix = (disk >= 26) ?
> > > > +disk_to_vbd_name((disk / 26) - 1) : g_strdup("");
> > > > +
> > > > +name = g_strdup_printf("%s%c", prefix, 'a' + disk);
> > >
> > > I don't think that works, if disk is 27, we do ('a' + 27) here. It's
> > > probably missing a `disk % 26`.
> >
> > Damn, yes I was not allowing the >2 letters.
> >
> > >
> > > > +g_free(prefix);
> > > > +
> > > > +return name;
> > > > +}
> > >
> > > [...]
> > >
> > > > +static unsigned int vbd_name_to_disk(const char *name, const char
> > > **endp)
> > > > +{
> > > > +unsigned int disk = 0;
> > > > +
> > > > +while (*name != '\0') {
> > > > +if (!g_ascii_isalpha(*name) || !g_ascii_islower(*name)) {
> > > > +break;
> > > > +}
> > > > +
> > > > +disk *= 26;
> > > > +disk += *name++ - 'a';
> > > > +}
> > > > +*endp = name;
> > > > +
> > > > +return disk;
> > > > +}
> > > > +
> > > > +static void xen_block_set_vdev(Object *obj, Visitor *v, const char
> > > *name,
> > > > + void *opaque, Error **errp)
> > > > +{
> > >
> > > Setting vdev doesn't work. I've tried to add a disk `xvdaa', and it
> > > result in `xvda', or `d0p0' (in the trace). (Same result with
> `xvdaaa',
> > > and 'xvdba' gives 'xvdaa'/d26p0)
> > >
> >
> > Ok, that's weird. I'll have to figure that out.
>
> It's probably because 'a' is somtime 0 and sometime is 1.
>
> 'a' should be 0
> 'aa' should be 26,
> 'aaa' Seems to be 702.
>
> 'xvda': 0 -> 0 * 1
> 'xvdz': 25->25 * 1
> 'xvdaa': 26 ->1 * 26 + 0 * 1
> 'xvdaaa': 702 -> 1 * 26^2 + 1 * 26 + 0 * 1
>
> So, it's weird. Have fun fixing the algorithm for that.
Actually not as hard as it seemed at first... just treat 'a' as 1 while
accumulating the number and then subtract 1 from the result. I wrote a tool to
generate the first N disk names using a simple "overflow 'z' to 'aa'" (i.e.
non-arithmetical) rule to check against and it matches all the cases I tried
(up to 4 letters).
Paul
>
> --
> Anthony PERARD
Re: [Qemu-devel] [PATCH for-4.0 0/6] vhost-user-blk: Add support for backend reconnecting
On 2018/12/6 下午9:59, Michael S. Tsirkin wrote: On Thu, Dec 06, 2018 at 09:57:22PM +0800, Jason Wang wrote: On 2018/12/6 下午2:35,elohi...@gmail.com wrote: From: Xie Yongji This patchset is aimed at supporting qemu to reconnect vhost-user-blk backend after vhost-user-blk backend crash or restart. The patch 1 tries to implenment the sync connection for "reconnect socket". The patch 2 introduces a new message VHOST_USER_SET_VRING_INFLIGHT to support offering shared memory to backend to record its inflight I/O. The patch 3,4 are the corresponding libvhost-user patches of patch 2. Make libvhost-user support VHOST_USER_SET_VRING_INFLIGHT. The patch 5 supports vhost-user-blk to reconnect backend when connection closed. The patch 6 tells qemu that we support reconnecting now. To use it, we could start qemu with: qemu-system-x86_64 \ -chardev socket,id=char0,path=/path/vhost.socket,reconnect=1,wait \ -device vhost-user-blk-pci,chardev=char0 \ and start vhost-user-blk backend with: vhost-user-blk -b /path/file -s /path/vhost.socket Then we can restart vhost-user-blk at any time during VM running. I wonder whether or not it's better to handle this at the level of virtio protocol itself instead of vhost-user level. E.g expose last_avail_idx to driver might be sufficient? Another possible issue is, looks like you need to deal with different kinds of ring layouts e.g packed virtqueues. Thanks I'm not sure I understand your comments here. All these would be guest-visible extensions. Looks not, it only introduces a shared memory between qemu and vhost-user backend? Possible for sure but how is this related to a patch supporting transparent reconnects? I might miss something. My understanding is that we support transparent reconnects, but we can't deduce an accurate last_avail_idx and this is what capability this series try to add. To me, this series is functional equivalent to expose last_avail_idx (or avail_idx_cons) in available ring. So the information is inside guest memory, vhost-user backend can access it and update it directly. I believe this is some modern NIC did as well (but index is in MMIO area of course). Thanks
Re: [Qemu-devel] [Qemu-ppc] [PATCH qemu] ppc/spapr: Receive and store device tree blob from SLOF
On Mon, 10 Dec 2018 17:20:43 +1100
David Gibson wrote:
> On Mon, Nov 12, 2018 at 03:12:26PM +1100, Alexey Kardashevskiy wrote:
> >
> >
> > On 12/11/2018 05:10, Greg Kurz wrote:
> > > Hi Alexey,
> > >
> > > Just a few remarks. See below.
> > >
> > > On Thu, 8 Nov 2018 12:44:06 +1100
> > > Alexey Kardashevskiy wrote:
> > >
> > >> SLOF receives a device tree and updates it with various properties
> > >> before switching to the guest kernel and QEMU is not aware of any changes
> > >> made by SLOF. Since there is no real RTAS (QEMU implements it), it makes
> > >> sense to pass the SLOF final device tree to QEMU to let it implement
> > >> RTAS related tasks better, such as PCI host bus adapter hotplug.
> > >>
> > >> Specifially, now QEMU can find out the actual XICS phandle (for PHB
> > >> hotplug) and the RTAS linux,rtas-entry/base properties (for firmware
> > >> assisted NMI - FWNMI).
> > >>
> > >> This stores the initial DT blob in the sPAPR machine and replaces it
> > >> in the KVMPPC_H_UPDATE_DT (new private hypercall) handler.
> > >>
> > >> This adds an @update_dt_enabled machine property to allow backward
> > >> migration.
> > >>
> > >> SLOF already has a hypercall since
> > >> https://github.com/aik/SLOF/commit/e6fc84652c9c0073f9183
> > >>
> > >> Signed-off-by: Alexey Kardashevskiy
> > >> ---
> > >> include/hw/ppc/spapr.h | 7 ++-
> > >> hw/ppc/spapr.c | 29 -
> > >> hw/ppc/spapr_hcall.c | 32
> > >> hw/ppc/trace-events| 2 ++
> > >> 4 files changed, 68 insertions(+), 2 deletions(-)
> > >>
> > >> diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
> > >> index ad4d7cfd97..f5dcaf44cb 100644
> > >> --- a/include/hw/ppc/spapr.h
> > >> +++ b/include/hw/ppc/spapr.h
> > >> @@ -100,6 +100,7 @@ struct sPAPRMachineClass {
> > >>
> > >> /*< public >*/
> > >> bool dr_lmb_enabled; /* enable dynamic-reconfig/hotplug of
> > >> LMBs */
> > >> +bool update_dt_enabled;/* enable KVMPPC_H_UPDATE_DT */
> > >> bool use_ohci_by_default; /* use USB-OHCI instead of XHCI */
> > >> bool pre_2_10_has_unused_icps;
> > >> bool legacy_irq_allocation;
> > >> @@ -136,6 +137,9 @@ struct sPAPRMachineState {
> > >> int vrma_adjust;
> > >> ssize_t rtas_size;
> > >> void *rtas_blob;
> > >> +uint32_t fdt_size;
> > >> +uint32_t fdt_initial_size;
> > >
> > > I don't quite see the purpose of fdt_initial_size... it seems to be only
> > > used to print a trace.
> >
> >
> > Ah, lost in rebase. The purpose was to test if the new device tree has
> > not grown too much.
> >
> >
> >
> > >
> > >> +void *fdt_blob;
> > >> long kernel_size;
> > >> bool kernel_le;
> > >> uint32_t initrd_base;
> > >> @@ -462,7 +466,8 @@ struct sPAPRMachineState {
> > >> #define KVMPPC_H_LOGICAL_MEMOP (KVMPPC_HCALL_BASE + 0x1)
> > >> /* Client Architecture support */
> > >> #define KVMPPC_H_CAS(KVMPPC_HCALL_BASE + 0x2)
> > >> -#define KVMPPC_HCALL_MAXKVMPPC_H_CAS
> > >> +#define KVMPPC_H_UPDATE_DT (KVMPPC_HCALL_BASE + 0x3)
> > >> +#define KVMPPC_HCALL_MAXKVMPPC_H_UPDATE_DT
> > >>
> > >> typedef struct sPAPRDeviceTreeUpdateHeader {
> > >> uint32_t version_id;
> > >> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> > >> index c08130facb..5e2d4d211c 100644
> > >> --- a/hw/ppc/spapr.c
> > >> +++ b/hw/ppc/spapr.c
> > >> @@ -1633,7 +1633,10 @@ static void spapr_machine_reset(void)
> > >> /* Load the fdt */
> > >> qemu_fdt_dumpdtb(fdt, fdt_totalsize(fdt));
> > >> cpu_physical_memory_write(fdt_addr, fdt, fdt_totalsize(fdt));
> > >> -g_free(fdt);
> > >> +g_free(spapr->fdt_blob);
> > >> +spapr->fdt_size = fdt_totalsize(fdt);
> > >> +spapr->fdt_initial_size = spapr->fdt_size;
> > >> +spapr->fdt_blob = fdt;
> > >
> > > Hmm... It looks weird to store state in a reset handler. I'd rather zeroe
> > > both fdt_blob and fdt_size here.
> >
> > The device tree is built from the reset handler and the idea is that we
> > want to always have some tree in the machine.
>
> Yes, I think the approach here is fine. Otherwise when we want to
> look up the current fdt state in RTAS calls or whatever we'd always
> have to do
> if (fdt_blob)
> look up that
> else
> look up qemu created fdt.
>
No. We only have one fdt blob: the initial one, I'd rather
call reset time one, or the updated one.
> Incidentally 'fdt' and 'fdt_blob' names do a terrible job of
> distinguishing what the difference is. Renaming fdt to fdt_initial
> (to match fdt_initial_size) and fdt_blob to fdt should make that
> clearer.
>
As mentioned earlier in this thread, spapr->fdt_initial_size is only used
for tracing if the received fdt blob fails fdt_check_full()...
$ git grep -H fdt_initial_size
hw/ppc/spapr.c:spapr->fdt_initial_size = spapr->fdt_size;
hw/ppc/spapr.c:VMSTATE_UINT32(fdt_initia
Re: [Qemu-devel] [PATCH 0/6] fw_cfg: add HMP 'info fw_cfg' and add_file_from_host()
On 12/7/18 11:48 PM, no-re...@patchew.org wrote:
> This series failed the docker-quick@centos7 build test. Please find the
> testing commands and
> their output below. If you have Docker installed, you can probably reproduce
> it
> locally.
>
...
> The full log is available at
> http://patchew.org/logs/20181207170400.5129-1-phi...@redhat.com/testing.docker-quick@centos7/?type=message.
qemu-system-x86_64: Back to tcg accelerator
Broken pipe
/tmp/qemu-test/src/tests/libqtest.c:125: kill_qemu() detected QEMU
death from signal 11 (Segmentation fault) (core dumped)
GTester: last random seed: R02Sb6a4400485731f23217a8855e030b450
Broken pipe
Probably the error Michael noticed:
>> +void hmp_info_fw_cfg(Monitor *mon, const QDict *qdict)
>> +{
>> +FWCfgState *s = fw_cfg_find();
>> +int arch, key;
>
> Looks like this will crash on a machine without fw cfg.
Re: [Qemu-devel] [PATCH 5/6] hw/nvram/fw_cfg: Add HMP 'info fw_cfg' command
On 12/7/18 6:54 PM, Michael S. Tsirkin wrote:
> On Fri, Dec 07, 2018 at 06:03:59PM +0100, Philippe Mathieu-Daudé wrote:
>> $ qemu-system-x86_64 -S -monitor stdio
>> (qemu) info fw_cfg
>> TypePermSizeSpecific Order Info
>
> Can we do better than "Info"?
For some entry this is the "content", but for the files this is the
"path". Do you prefer "Content or path"?
>
>>signature RO 4 QEMU
>> id RO 4 0x0003
>> uuid RO 16
>> ----
>> ram_size RO 8 0x0800
>>nographic RO 2 0x
>> nb_cpus RO 2 0x0001
>> numa RO 16
>>boot_menu RO 2 0x
>> max_cpus RO 2 0x0001
>> file_dir RO2052
>> file (id 1) RO 36 160 etc/acpi/rsdp
>> file (id 2) RO 131072 130 etc/acpi/tables
>> file (id 3) RO 4 15 etc/boot-fail-wait
>> file (id 4) RO 20 40 etc/e820
>> file (id 5) RO 31 30 etc/smbios/smbios-anchor
>> file (id 6) RO 320 20 etc/smbios/smbios-tables
>> file (id 7) RO 6 90 etc/system-states
>> file (id 8) RO4096 140 etc/table-loader
>> file (id 10) RO9216 55 genroms/kvmvapic.bin
>> uuid RO 4 (arch spec)
>> 0100----
>> ram_size RO 324 (arch spec)
>>nographic RO 121 (arch spec)
>
> Weird. Your code has arch_spec.
Hmmm I'll check that.
>
>> (qemu)
>>
>> Signed-off-by: Philippe Mathieu-Daudé
>
>
> Looks helpful but generally IMHO whatever is exposed through hmp
> should be in the qmp as well.
OK.
>
>
>> ---
>> hmp-commands-info.hx | 14 +
>> hw/nvram/fw_cfg.c | 115 ++
>> include/hw/nvram/fw_cfg.h | 2 +
>> 3 files changed, 131 insertions(+)
>>
>> diff --git a/hmp-commands-info.hx b/hmp-commands-info.hx
>> index cbee8b944d..9e86dceeb4 100644
>> --- a/hmp-commands-info.hx
>> +++ b/hmp-commands-info.hx
>> @@ -916,6 +916,20 @@ STEXI
>> @item info sev
>> @findex info sev
>> Show SEV information.
>> +ETEXI
>> +
>> +{
>> +.name = "fw_cfg",
>> +.args_type = "",
>> +.params = "",
>> +.help = "Display the table of the fw_cfg entries registered",
>
> I think the help line should be a bit more verbose.
> Mention it's a paravirtualized interface, and why
> would one want to display it (debugging guest firmware?).
OK.
>
>
>> +.cmd= hmp_info_fw_cfg,
>> +},
>> +
>> +STEXI
>> +@item info fw_cfg
>> +@findex info fw_cfg
>> +Show roms.
>> ETEXI
>>
>> STEXI
>> diff --git a/hw/nvram/fw_cfg.c b/hw/nvram/fw_cfg.c
>> index 582653f07e..50525ec1dd 100644
>> --- a/hw/nvram/fw_cfg.c
>> +++ b/hw/nvram/fw_cfg.c
>> @@ -36,6 +36,7 @@
>> #include "qemu/config-file.h"
>> #include "qemu/cutils.h"
>> #include "qapi/error.h"
>> +#include "monitor/monitor.h"
>>
>> #define FW_CFG_FILE_SLOTS_DFLT 0x20
>>
>> @@ -1164,3 +1165,117 @@ static void fw_cfg_register_types(void)
>> }
>>
>> type_init(fw_cfg_register_types)
>> +
>> +static const char *fw_cfg_wellknown_entries[0x20] = {
>> +[FW_CFG_SIGNATURE] = "signature",
>> +[FW_CFG_ID] = "id",
>> +[FW_CFG_UUID] = "uuid",
>> +[FW_CFG_RAM_SIZE] = "ram_size",
>> +[FW_CFG_NOGRAPHIC] = "nographic",
>> +[FW_CFG_NB_CPUS] = "nb_cpus",
>> +[FW_CFG_MACHINE_ID] = "machine_id",
>> +[FW_CFG_KERNEL_ADDR] = "kernel_addr",
>> +[FW_CFG_KERNEL_SIZE] = "kernel_size",
>> +[FW_CFG_KERNEL_CMDLINE] = "kernel_cmdline",
>> +[FW_CFG_INITRD_ADDR] = "initrd_addr",
>> +[FW_CFG_INITRD_SIZE] = "initdr_size",
>> +[FW_CFG_BOOT_DEVICE] = "boot_device",
>> +[FW_CFG_NUMA] = "numa",
>> +[FW_CFG_BOOT_MENU] = "boot_menu",
>> +[FW_CFG_MAX_CPUS] = "max_cpus",
>> +[FW_CFG_KERNEL_ENTRY] = "kernel_entry",
>> +[FW_CFG_KERNEL_DATA] = "kernel_data",
>> +[FW_CFG_INITRD_DATA] = "initrd_data",
>> +[FW_CFG_CMDLINE_ADDR] = "cmdline_addr",
>> +[FW_CFG_CMDLINE_SIZE] = "cmdline_size",
>> +[FW_CFG_CMDLINE_DATA] = "cmdline_data",
>> +[FW_CFG_SETUP_ADDR] = "setup_addr",
>> +[FW_CFG_SETUP_SIZE] = "setup_size",
>> +[FW_CFG_SETUP_DATA] = "setup_data",
>> +[FW_CFG_FILE_DIR] = "file_dir",
>> +};
>> +
>> +void hmp_info_fw_cfg(Monitor *mon, const QDict *qdict)
>> +{
>> +FWCfgState *s = fw_cfg_find();
>> +int arch, key;
>
> Looks like this will crash on a machine without fw cfg.
Oops, good catch :)
>
>
>> +
>> +monitor_printf(mon, "%12s %5s %7s %9s %6s %-24s\n",
>> +
Re: [Qemu-devel] [PATCH 2/6] hw/arm: Remove unused include
Hi Michael, Peter. On 12/7/18 6:57 PM, Michael S. Tsirkin wrote: > On Fri, Dec 07, 2018 at 06:03:56PM +0100, Philippe Mathieu-Daudé wrote: >> Signed-off-by: Philippe Mathieu-Daudé >> --- >> hw/arm/virt-acpi-build.c | 1 - >> 1 file changed, 1 deletion(-) >> >> diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c >> index 5785fb697c..98d7f7cf20 100644 >> --- a/hw/arm/virt-acpi-build.c >> +++ b/hw/arm/virt-acpi-build.c >> @@ -35,7 +35,6 @@ >> #include "target/arm/cpu.h" >> #include "hw/acpi/acpi-defs.h" >> #include "hw/acpi/acpi.h" >> -#include "hw/nvram/fw_cfg.h" >> #include "hw/acpi/bios-linker-loader.h" >> #include "hw/loader.h" >> #include "hw/hw.h" > > > This file uses fw_cfg_add_file, looks wrong. Previously I thought if another include (here hw/loader.h) already pulls this header, we can drop it, but Peter Maydell explained me this not safe for refactors: Generally I think that if a .c file directly uses function X declared in header Y it should #include Y, even if it happens that it already includes other header Z that includes Y. Otherwise if we refactor Z later such that it no longer needs to include Y, it will break compilation of the .c file. (That is, Z including Y is a detail of the implementation of Z, not a guarantee made by Z to its users.) https://lists.gnu.org/archive/html/qemu-devel/2018-11/msg02460.html I'm sorry I forgot to update this in the series :/ Regards, Phil.
[Qemu-devel] [Bug 1803160] Re: qemu-3.1.0-rc0: tcg.c crash in temp_load
I've just opened #1807675 for the new bug. Thanks! -- You received this bug notification because you are a member of qemu- devel-ml, which is subscribed to QEMU. https://bugs.launchpad.net/bugs/1803160 Title: qemu-3.1.0-rc0: tcg.c crash in temp_load Status in QEMU: Fix Committed Bug description: QEMU version: - qemu-3.1.0-rc0 compiled from sources (earlier versions also affected) Summary: TCG crashes in i386 and x86_64 when it tries to execute some specific illegal instructions. When running full OS emulation, both the guest system and QEMU crash. The issue has been reproduced in two scenarios: Ubuntu x64 host running Debian x86 guest with the following command line: qemu-system-x86_64 -m 4G debian.qcow When the attached ELF file is executed inside the guest, QEMU crashes. It can also be reproduced from the command line: $ qemu-i386 tcg_crash.elf /home/alberto/Documents/qemu-3.1.0-rc0/tcg/tcg.c:2863: tcg fatal error qemu: uncaught target signal 11 (Segmentation fault) - core dumped zsh: segmentation fault (core dumped) ../qemu-3.1.0-rc0/build/i386-linux-user/qemu-i386 tcg_crash.elf GDB backtrace: (gdb) bt #0 0x60206488 in raise () #1 0x60206b8a in abort () #2 0x60007016 in temp_load (s=s@entry=0x607a2780 , ts=ts@entry=0x607a3178 , desired_regs=, allocated_regs=allocated_regs@entry=16400) at /home/alberto/Documents/qemu-3.1.0-rc0/tcg/tcg.c:2863 #3 0x6000a4d9 in tcg_reg_alloc_op (op=0x62808c20, s=) at /home/alberto/Documents/qemu-3.1.0-rc0/tcg/tcg.c:3070 #4 tcg_gen_code (s=, tb=tb@entry=0x607ac040 ) at /home/alberto/Documents/qemu-3.1.0-rc0/tcg/tcg.c:3598 #5 0x6003ef9a in tb_gen_code (cpu=cpu@entry=0x627e0010, pc=pc@entry=134512724, cs_base=cs_base@entry=0, flags=flags@entry=4194483, cflags=cflags@entry=0) at /home/alberto/Documents/qemu-3.1.0-rc0/accel/tcg/translate-all.c:1752 #6 0x6003d979 in tb_find (cf_mask=0, tb_exit=0, last_tb=0x0, cpu=0x0) at /home/alberto/Documents/qemu-3.1.0-rc0/accel/tcg/cpu-exec.c:404 #7 cpu_exec (cpu=cpu@entry=0x627e0010) at /home/alberto/Documents/qemu-3.1.0-rc0/accel/tcg/cpu-exec.c:724 #8 0x6006e1a0 in cpu_loop (env=env@entry=0x627e82c0) at /home/alberto/Documents/qemu-3.1.0-rc0/linux-user/i386/cpu_loop.c:93 #9 0x600037c5 in main (argc=2, argv=0x7fffdd28, envp=) at /home/alberto/Documents/qemu-3.1.0-rc0/linux-user/main.c:819 (gdb) Testcase: - Find ELF file attached. To manage notifications about this bug go to: https://bugs.launchpad.net/qemu/+bug/1803160/+subscriptions
[Qemu-devel] [Bug 1807675] [NEW] qemu commit 80422b0: tcg.c crash in temp_load
Public bug reported: As discussed in #1803160 I'm opening a new ticket for the new bug. QEMU version: - qemu from git, master branch commit 80422b00196a7af4c6efb628fae0ad8b644e98af Summary: TCG crashes in i386 and x86_64 when it tries to execute some specific illegal instructions. When running full OS emulation, both the guest system and QEMU crash. $ qemu-i386 tcg_crash1.elf /home/alberto/Documents/qemu/tcg/tcg.c:2863: tcg fatal error qemu: uncaught target signal 11 (Segmentation fault) - core dumped zsh: segmentation fault (core dumped) ./qemu/build/i386-linux-user/qemu-i386 tcg_crash1.elf Invalid instructions: f0 invalid 40 inc eax a7 cmpsd dword [esi], dword ptr es:[edi] 48 dec eax Testcase: - Find ELF file attached. ** Affects: qemu Importance: Undecided Status: New ** Tags: tcg ** Attachment added: "tcg_crash1.elf" https://bugs.launchpad.net/bugs/1807675/+attachment/5220847/+files/tcg_crash1.elf -- You received this bug notification because you are a member of qemu- devel-ml, which is subscribed to QEMU. https://bugs.launchpad.net/bugs/1807675 Title: qemu commit 80422b0: tcg.c crash in temp_load Status in QEMU: New Bug description: As discussed in #1803160 I'm opening a new ticket for the new bug. QEMU version: - qemu from git, master branch commit 80422b00196a7af4c6efb628fae0ad8b644e98af Summary: TCG crashes in i386 and x86_64 when it tries to execute some specific illegal instructions. When running full OS emulation, both the guest system and QEMU crash. $ qemu-i386 tcg_crash1.elf /home/alberto/Documents/qemu/tcg/tcg.c:2863: tcg fatal error qemu: uncaught target signal 11 (Segmentation fault) - core dumped zsh: segmentation fault (core dumped) ./qemu/build/i386-linux-user/qemu-i386 tcg_crash1.elf Invalid instructions: f0 invalid 40 inc eax a7 cmpsd dword [esi], dword ptr es:[edi] 48 dec eax Testcase: - Find ELF file attached. To manage notifications about this bug go to: https://bugs.launchpad.net/qemu/+bug/1807675/+subscriptions
Re: [Qemu-devel] possible bug hw/adc/stm32f2xx_adc.c
Hi Seth,
On 12/10/18 4:39 AM, Seth K wrote:
> Thank you all for help with my last patch. I found one more entry in my
> notes that could be a bug, or could be a misunderstanding on my part.
>
> The memory map in DocID15818 (Rev 15) datasheet says:
> ADC1 - ADC2 - ADC3: 0x40012000-0x400123FF
>
> That suggests a size of 0x400 (they share that range?)
This datasheet is more about how the chip interracts with the external
world. There is another document about how to use it (internally) which
is the "Technical Reference Manual" (RM0033 Rev 8):
https://www.st.com/resource/en/reference_manual/cd00225773.pdf
See Section 10.13.18 "ADC register map": There are 3 ADCs indeed, each
mapped at a 0x100 base. Each ADC can be addressed in a 0x50 bytes range.
>
> Line 279/280 of hw/adc/stm32f2xx_adc.c seems to use 0xFF
> memory_region_init_io(&s->mmio,obj,&stm32f2xx_adc_ops,s,
> TYPE_STM32F2XX_ADC,0xFF);
This code define a single ADC and looks correct.
The consumer of this ADC is the SoC object, defined in the file
hw/arm/stm32f205_soc.c. You can see lines 70:
for (i = 0; i < STM_NUM_ADCS; i++) {
sysbus_init_child_obj(obj, "adc[*]", &s->adc[i], sizeof(s->adc[i]),
TYPE_STM32F2XX_ADC);
}
Having in include/hw/arm/stm32f205_soc.h:42:#define STM_NUM_ADCS 3
and:
static const uint32_t adc_addr[STM_NUM_ADCS] = { 0x40012000, 0x40012100,
0x40012200 };
So the SoC code instanciates 3 ADCs, each at the correct address.
(You can check the SoC Memory Map at Section 2.3 "Memory map" p. 53).
An easy way to figure out the memory map in QEMU is using the 'info
mtree' in the QEMU Monitor:
qemu-system-arm -M netduino2 -kernel /dev/null -S -monitor stdio
QEMU 3.0.94 monitor - type 'help' for more information
(qemu) info mtree
address-space: memory
- (prio -1, i/o): system
-000f (prio 0, i/o): alias flash
0800-080f (prio 0, rom): STM32F205.flash
2000-2001 (prio 0, ram): STM32F205.sram
4000-43ff (prio 0, i/o): stm32f2xx_timer
4400-47ff (prio 0, i/o): stm32f2xx_timer
4800-4bff (prio 0, i/o): stm32f2xx_timer
4c00-4fff (prio 0, i/o): stm32f2xx_timer
40003800-40003bff (prio 0, i/o): stm32f2xx-spi
40003c00-40003fff (prio 0, i/o): stm32f2xx-spi
40004400-400047ff (prio 0, i/o): stm32f2xx-usart
40004800-40004bff (prio 0, i/o): stm32f2xx-usart
40004c00-40004fff (prio 0, i/o): stm32f2xx-usart
40005000-400053ff (prio 0, i/o): stm32f2xx-usart
40011000-400113ff (prio 0, i/o): stm32f2xx-usart
40011400-400117ff (prio 0, i/o): stm32f2xx-usart
40012000-400120fe (prio 0, i/o): stm32f2xx-adc
40012100-400121fe (prio 0, i/o): stm32f2xx-adc
40012200-400122fe (prio 0, i/o): stm32f2xx-adc
40013000-400133ff (prio 0, i/o): stm32f2xx-spi
40013800-40013bff (prio 0, i/o): stm32f2xx-syscfg
You are correct we should declare a 0x100 region rather than 0xff.
>
> Probably just confusion on my part, but thought I would mention it just
> in case.
> Thanks,
> Seth
>
> PS: Sorry if you are all the wrong people to email about this ADC...
You did well :)
You can also use this script:
./scripts/get_maintainer.pl -f hw/adc/stm32f2xx_adc.c
Alistair Francis (maintainer:STM32F205)
Peter Maydell (maintainer:STM32F205)
qemu-devel@nongnu.org (open list:All patches CC here)
But for this particular case, using the SoC file is more complete (you
get the ARM list):
./scripts/get_maintainer.pl -f hw/arm/stm32f205_soc.c
Alistair Francis (maintainer:STM32F205)
Peter Maydell (maintainer:STM32F205)
qemu-...@nongnu.org (open list:ARM)
qemu-devel@nongnu.org (open list:All patches CC here)
Regards,
Phil.
Re: [Qemu-devel] [PATCH v6 15/27] qapi: rename allow_dict to allow_implicit
Marc-André Lureau writes:
> On Wed, Dec 5, 2018 at 10:41 PM Markus Armbruster wrote:
>>
>> Marc-André Lureau writes:
>>
>> > This makes it a bit clearer what is the intent of the dictionnary for
>>
>> dictionary
>
> sigh, this must be a very common misspell (dictionnaire in french)
Muscle memory...
>> > the check_type() function, since there was some confusion on a
>> > previous iteration of this series.
>>
>> I don't remember... got a pointer to that confusion handy?
>
> https://lists.nongnu.org/archive/html/qemu-devel/2017-12/msg01584.html
Thanks!
My review comment was
> diff --git a/scripts/qapi.py b/scripts/qapi.py
> index df2a304e8f..15711f96fa 100644
> --- a/scripts/qapi.py
> +++ b/scripts/qapi.py
> @@ -696,7 +696,15 @@ def check_type(info, source, value,
allow_array=False,
> return
>
> if not allow_dict:
> -raise QAPISemError(info, "%s should be a type name" % source)
> +if isinstance(value, dict) and 'type' in value:
> +check_type(info, source, value['type'], allow_array,
> + allow_dict, allow_optional, allow_metas)
> +if 'if' in value:
> +check_if(value, info)
> +check_unknown_keys(info, value, {'type', 'if'})
> +return
> +else:
> +raise QAPISemError(info, "%s should be a type name" % source)
@allow_dict becomes a misnomer: dictionaries are now always allowed, but
they have different meaning (implicit type vs. named type with
additional attributes).
Rename to @allow_implicit?
As far as I can tell, it's not an issue in the current version of your
patches anymore:
def check_type(info, source, value, allow_array=False,
allow_implicit=False, allow_optional=False,
allow_metas=[]):
global all_names
if value is None:
return
# Check if array type for value is okay
if isinstance(value, list):
if not allow_array:
raise QAPISemError(info, "%s cannot be an array" % source)
if len(value) != 1 or not isinstance(value[0], str):
raise QAPISemError(info,
"%s: array type must contain single type
name" %
source)
value = value[0]
# Check if type name for value is okay
if isinstance(value, str):
if value not in all_names:
raise QAPISemError(info, "%s uses unknown type '%s'"
% (source, value))
if not all_names[value] in allow_metas:
raise QAPISemError(info, "%s cannot use %s type '%s'" %
(source, all_names[value], value))
return
if not allow_implicit:
raise QAPISemError(info, "%s should be a type name" % source)
if not isinstance(value, OrderedDict):
raise QAPISemError(info,
"%s should be a dictionary or type name" %
source)
# value is a dictionary, check that each member is okay
for (key, arg) in value.items():
check_name(info, "Member of %s" % source, key,
allow_optional=allow_optional)
if c_name(key, False) == 'u' or c_name(key,
False).startswith('has_'):
raise QAPISemError(info, "Member of %s uses reserved name '%s'"
% (source, key))
# Todo: allow dictionaries to represent default values of
# an optional argument.
if isinstance(arg, dict):
check_known_keys(info, "member '%s' of %s" % (key, source),
arg, ['type'], ['if'])
typ = arg['type']
else:
typ = arg
check_type(info, "Member '%s' of %s" % (key, source),
typ, allow_array=True,
allow_metas=['built-in', 'union', 'alternate', 'struct',
'enum'])
>> > Suggested-by: Markus Armbruster
>> > Signed-off-by: Marc-André Lureau
