Re: [Qemu-devel] [PATCH v4 00/19] reverse debugging
> From: Alex Bennée [mailto:alex.ben...@linaro.org] > Pavel Dovgalyuk writes: > > > Ping? > > I started having a look but I ran into this straight away. First I > recorded a boot of the kernel: > > ./aarch64-softmmu/qemu-system-aarch64 -machine virt,graphics=on,gic- > version=3,virtualization=on -cpu cortex-a53 --serial mon:stdio -display none > -kernel > ../images/aarch64-current-linux-initrd-guest.img -icount > shift=7,rr=record,rrfile=replay.bin > > Then played back: > > ./aarch64-softmmu/qemu-system-aarch64 -machine virt,graphics=on,gic- > version=3,virtualization=on -cpu cortex-a53 --serial mon:stdio -display none > -kernel > ../images/aarch64-current-linux-initrd-guest.img -icount > shift=7,rr=replay,rrfile=replay.bin - > s -S This looks ok, but... > And did the following on gdb: > > (gdb) i > 0x4004 in ?? () > => 0x4004: mov x1, xzr >0x4008: mov x2, xzr >0x400c: mov x3, xzr > (gdb) > 0x4008 in ?? () > => 0x4008: mov x2, xzr >0x400c: mov x3, xzr >0x4010: ldr x4, 0x4020 > (gdb) > 0x400c in ?? () > => 0x400c: mov x3, xzr >0x4010: ldr x4, 0x4020 >0x4014: br x4 > (gdb) > 0x4010 in ?? () > => 0x4010: ldr x4, 0x4020 >0x4014: br x4 >0x4018: .inst 0x4400 ; undefined > (gdb) > 0x4014 in ?? () > => 0x4014: br x4 >0x4018: .inst 0x4400 ; undefined >0x401c: .inst 0x ; undefined > (gdb) p/x $x4 > $1 = 0x4008 > (gdb) reverse-stepi > warning: Remote failure reply: E14 > > Surely this is the simple case and doesn't require any snapshots for > block devices as there are none. Am I missing something? Reverse debugging requires the snapshotting. QEMU can't revert the VM state without the snapshots. You can try adding an empty qcow2 image to allow snapshotting there. Pavel Dovgalyuk
Re: [Qemu-devel] [PATCH 6/6] linux-user: Use *at functions to implement interp_prefix
On 06/03/2018 06:04 PM, Laurent Vivier wrote: > On 01/06/2018 00:49, Richard Henderson wrote: >> If the interp_prefix is a complete chroot, it may have a *lot* of files. >> Setting up the cache for this is quite expensive. >> >> For the most part, we can use the *at versions of various syscalls to >> attempt the operation in the prefix. For the few cases that remain, >> attempt the operation in the prefix via concatenation and then retry >> if that fails. >> > > I like the idea, but it breaks real chroot. > > You can test it with: > > wget > https://github.com/vivier/linux-user-test-scrips/raw/master/create_chroot.sh > > then > > sudo sh ./create_chroot.sh /path/to/static/qemu-s390x stretch *shrug* Works for me, at least as far as I can test. Your script doesn't work outside debian, lacking debootstrap. At the moment, I can't build on debian *at all*. Some bit of the build infrastructure is off and QEMU_FULL_VERSION gets incorrectly defined. I have no idea how or why it is different than Fedora. On Fedora 28, one can no longer build a static qemu. We depend on libraries for which Fedora no longer ships static versions. Do you have some specific binary that fails? r~
Re: [Qemu-devel] [PATCH] CODING_STYLE: Define our preferred form for multiline comments
On 05.06.2018 03:17, Alex Williamson wrote: > On Mon, 4 Jun 2018 17:21:40 +0100 > Peter Maydell wrote: > >> The codebase has a bit of a mix of >> /* multiline comments >> * like this >> */ >> and >> /* multiline comments like this >> in the GNU Coding Standards style */ >> >> State a preference for the former. >> >> Signed-off-by: Peter Maydell >> --- >> I admit that to some extent I'm imposing my aesthetic >> preferences here; pretty sure we have a lot more style >> 1 comments than style 2, though. >> --- >> CODING_STYLE | 13 + >> 1 file changed, 13 insertions(+) >> >> diff --git a/CODING_STYLE b/CODING_STYLE >> index 12ba58ee293..fb1d1f1cd62 100644 >> --- a/CODING_STYLE >> +++ b/CODING_STYLE >> @@ -124,6 +124,19 @@ We use traditional C-style /* */ comments and avoid // >> comments. >> Rationale: The // form is valid in C99, so this is purely a matter of >> consistency of style. The checkpatch script will warn you about this. >> >> +Multiline comments blocks should have a row of stars on the left >> +and the terminating */ on its own line: >> +/* like >> + * this >> + */ >> +Putting the initial /* on its own line is accepted, but not required. > > Could we say "at maintainer discretion", or is that always implied? The > asymmetry of the proposed standard is not my favorite and a mostly > blank line before and after further supports standing out from > surrounding code. I also don't like the asymmetry. I'd prefer more dense comments, though: /* like * this */ Anyway, could we either use that dense format or the kernel-style multi-lines-comment format, please? Mixing it asymmetrically is just ugly. Thomas
[Qemu-devel] [Bug 1441443] Re: Is there a way to create a 10G network interface for VMs in KVM2.0?
[Expired for QEMU because there has been no activity for 60 days.] ** Changed in: qemu Status: Incomplete => Expired -- You received this bug notification because you are a member of qemu- devel-ml, which is subscribed to QEMU. https://bugs.launchpad.net/bugs/1441443 Title: Is there a way to create a 10G network interface for VMs in KVM2.0? Status in QEMU: Expired Bug description: We have installed & configured the KVM 2.0 (qemu-kvm 2.0.0+dfsg- 2ubuntu1.10) on Ubuntu 14.04. The physical server is connected to 10G network, KVM is configured in Bridged mode But the issue is, when we create Network interface on VMs, we have only 1G device as an options for vmhosts. Is this the limit of the KVM or is there a way to create a 10G network interface for VMs? Available device models E1000 Ne2k_pci Pcnet Rtl8139 virtio Please find the network configuration details Source device : Host device vnet1 (Bridge ‘br0’) Device model : virtio Network configuration in the host /etc/network/interfaces auto br0 iface br0 inet static address 10.221.x.10 netmask 255.255.255.0 network 10.221.x.0 broadcast 10.221.x.255 gateway 10.221.x.1 # dns-* options are implemented by the resolvconf package, if installed dns-nameservers X.X.X.X dns-search XXX.NET bridge_ports em1 bridge_fd 0 bridge_stp off bridge_maxwait 0 To manage notifications about this bug go to: https://bugs.launchpad.net/qemu/+bug/1441443/+subscriptions
[Qemu-devel] The side effect of changing Processor Brand string?
Hi All, I'd like to change the Processor Brand String(CPUID[0x8002|0x8003|0x8004]) of my guest OSS cpu model to a string that won't be standard Intel or AMD related processor brand string. Would this change have any side effect on the applications that run on this guest? Seems some application would check the Processor Brand String for choosing different code path, for eg Oracle database? Thanks, Lizhen
Re: [Qemu-devel] [PATCH 03/17] iotests: ask qemu for supported formats
On 05.06.2018 00:40, Eric Blake wrote: > On 06/04/2018 05:34 AM, Thomas Huth wrote: >> On 04.06.2018 09:18, Markus Armbruster wrote: >>> Roman Kagan writes: >>> Add helper functions to query the block drivers actually supported by QEMU using "-drive format=?". This allows to skip certain tests that require drivers not built in or whitelisted in QEMU. > + supported_formats=$($QEMU_PROG $QEMU_OPTIONS -drive format=\? 2>&1 | \ >>> >>> Use of '?' to get help is deprecated. Please use 'format=help', and >>> update your commit message accordingly. >> >> Is it? Where did we document that? > > Hmm, we haven't documented it yet, but it's been that way since commit > c8057f95, in Aug 2012, when we added 'help' as the preferred synonym, > and have tried to avoid mention of '?' in new documentation (as it > requires additional shell quoting). I'm guessing we'll probably see a > patch from you to start an official deprecation window? I'm using '?' regularly on my own, so don't expect a patch from my side here ;-) Anyway, we still use the question mark in our documentation, e.g.: options is a comma separated list of format specific options in a name=value format. Use -o ? for an overview of the options supported by the used format or see the format descriptions below for details. or: -b, --blacklist=list Comma-separated list of RPCs to disable (no spaces, ‘?’ to list available RPCs) So calling it deprecated sounds wrong to me. Thomas
Re: [Qemu-devel] [PATCH 2/4] sparp_pci: simplify how the PCI LSIs are allocated
On Sat, May 26, 2018 at 11:40:23AM +0200, Greg Kurz wrote:
> On Fri, 18 May 2018 18:44:03 +0200
> Cédric Le Goater wrote:
>
> > PCI LSIs are today allocated one by one using the IRQ alloc_block
> > routine. Change the code sequence to first allocate a PCI_NUM_PINS
> > block. It will help us providing a generic IRQ framework to the
> > machine.
> >
> > Signed-off-by: Cédric Le Goater
> > ---
> > hw/ppc/spapr_pci.c | 21 ++---
> > 1 file changed, 10 insertions(+), 11 deletions(-)
> >
> > diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
> > index 39a14980d397..4fd97ffe4c6e 100644
> > --- a/hw/ppc/spapr_pci.c
> > +++ b/hw/ppc/spapr_pci.c
> > @@ -1546,6 +1546,8 @@ static void spapr_phb_realize(DeviceState *dev, Error
> > **errp)
> > sPAPRTCETable *tcet;
> > const unsigned windows_supported =
> > sphb->ddw_enabled ? SPAPR_PCI_DMA_MAX_WINDOWS : 1;
> > +uint32_t irq;
> > +Error *local_err = NULL;
> >
> > if (!spapr) {
> > error_setg(errp, TYPE_SPAPR_PCI_HOST_BRIDGE " needs a pseries
> > machine");
> > @@ -1694,18 +1696,15 @@ static void spapr_phb_realize(DeviceState *dev,
> > Error **errp)
> > QLIST_INSERT_HEAD(>phbs, sphb, list);
> >
> > /* Initialize the LSI table */
> > -for (i = 0; i < PCI_NUM_PINS; i++) {
> > -uint32_t irq;
> > -Error *local_err = NULL;
> > -
> > -irq = spapr_irq_alloc_block(spapr, 1, true, false, _err);
> > -if (local_err) {
> > -error_propagate(errp, local_err);
> > -error_prepend(errp, "can't allocate LSIs: ");
> > -return;
> > -}
> > +irq = spapr_irq_alloc_block(spapr, PCI_NUM_PINS, true, false,
> > _err);
> > +if (local_err) {
> > +error_propagate(errp, local_err);
> > +error_prepend(errp, "can't allocate LSIs: ");
> > +return;
> > +}
> >
>
> It isn't strictly equivalent. The current code would be happy with
> sparse IRQ numbers, while the proposed one wouldn't... Anyway, this
> cannot happen since we don't have PHB hotplug.
This makes me pretty nervous, because it's not obvious it will come up
with the same numbers in all circumstances, which we have to do for
existing machine types. It's also not obvious to me why it's useful
to go via this step before going straight to static allocation of the
irq numbers.
If you can convince me this will (in practice) return the same numbers
as the existing code for all valid setups, and that it's a useful
intermediate step, then I'll apply it.
>
> > -sphb->lsi_table[i].irq = irq;
> > +for (i = 0; i < PCI_NUM_PINS; i++) {
> > +sphb->lsi_table[i].irq = irq + i;
> > }
> >
> > /* allocate connectors for child PCI devices */
>
--
David Gibson| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson
signature.asc
Description: PGP signature
Re: [Qemu-devel] [PATCH] monitor: postpone monitor_qmp_cleanup_queues
On Mon, Jun 04, 2018 at 05:01:21PM +0200, Marc-André Lureau wrote:
> Hi
>
> On Mon, Jun 4, 2018 at 10:06 AM, Peter Xu wrote:
> > Previously we cleanup the queues when we got CLOSED event. It was used
> > to make sure we won't leftover replies/events of a old client to a new
> > client. Now this patch postpones that until OPENED.
> >
> > In most cases, it does not really matter much since either way will make
> > sure that the new client won't receive unexpected old events/responses.
> > However there can be a very rare race condition with the old way when we
> > use QMP with stdio and pipelines (which is quite common in iotests).
> > The problem is that, we can easily have something like this in scripts:
> >
> > cat $QMP_COMMANDS | qemu -qmp stdio ... | filter_commands
> >
> > In most cases, a QMP session will be based on a bidirectional
> > channel (read/write to a TCP port, for example), so IN and OUT are
> > sharing some fundamentally same thing. However that's untrue for pipes
> > above - the IN is the "cat" program, while the "OUT" is directed to the
> > "filter_commands" which is actually another process.
> >
> > Now when we received the CLOSED event, we cleanup the queues (including
> > the QMP response queue). That means, if we kill/stop the "cat" process
> > faster than the filter_commands process, the filter_commands process is
> > possible to miss some responses/events that should belong to it.
> >
> > In practise, I encountered a very strange SHUTDOWN event missing when
> > running test with iotest 087. Without this patch, iotest 078 will have
> > ~10% chance to miss the SHUTDOWN event and fail when with Out-Of-Band
> > enabled:
> >
> > 087 8s ... - output mismatch (see 087.out.bad)
> > --- /home/peterx/git/qemu/tests/qemu-iotests/087.out2018-06-01
> > 18:44:22.378982462 +0800
> > +++ /home/peterx/git/qemu/bin/tests/qemu-iotests/087.out.bad2018-06-01
> > 18:53:44.267840928 +0800
> > @@ -8,7 +8,6 @@
> > {"return": {}}
> > {"error": {"class": "GenericError", "desc": "'node-name' must be specified
> > for the root node"}}
> > {"return": {}}
> > -{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP},
> > "event": "SHUTDOWN", "data": {"guest": false}}
> >
> > === Duplicate ID ===
> > @@ -53,7 +52,6 @@
> > {"return": {}}
> > {"return": {}}
> > {"return": {}}
> > -{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP},
> > "event": "SHUTDOWN", "data": {"guest": false}}
> >
> > This patch fixes the problem.
> >
> > Fixes: 6d2d563f8c ("qmp: cleanup qmp queues properly", 2018-03-27)
> > Signed-off-by: Peter Xu
> >
> > Signed-off-by: Peter Xu
> > ---
> > monitor.c | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/monitor.c b/monitor.c
> > index 46814af533..6f26108108 100644
> > --- a/monitor.c
> > +++ b/monitor.c
> > @@ -4354,6 +4354,7 @@ static void monitor_qmp_event(void *opaque, int event)
> >
> > switch (event) {
> > case CHR_EVENT_OPENED:
> > +monitor_qmp_cleanup_queues(mon);
> > mon->qmp.commands = _cap_negotiation_commands;
> > monitor_qmp_caps_reset(mon);
> > data = get_qmp_greeting(mon);
> > @@ -4362,7 +4363,6 @@ static void monitor_qmp_event(void *opaque, int event)
> > mon_refcount++;
> > break;
> > case CHR_EVENT_CLOSED:
> > -monitor_qmp_cleanup_queues(mon);
> > json_message_parser_destroy(>qmp.parser);
> > json_message_parser_init(>qmp.parser, handle_qmp_command);
> > mon_refcount--;
> > --
> > 2.17.0
> >
> >
>
> Drawback, we will not clean up pending commands until next client.
IMHO that's fine.
>
> Perhaps we could have something more specific to the stdio case (untested):
Yeah if we can fix that from the QIO layer that'll be good to me too,
though...
>
>
> diff --git a/include/io/channel-file.h b/include/io/channel-file.h
> index ebfe54ec70..04a20bc2ed 100644
> --- a/include/io/channel-file.h
> +++ b/include/io/channel-file.h
> @@ -90,4 +90,12 @@ qio_channel_file_new_path(const char *path,
>mode_t mode,
>Error **errp);
>
> +/**
> + * qio_channel_file_is_opened:
> + *
> + * Returns: true if the associated file descriptor is valid & opened.
> + */
> +bool
> +qio_channel_file_is_opened(QIOChannelFile *ioc);
> +
> #endif /* QIO_CHANNEL_FILE_H */
> diff --git a/chardev/char-fd.c b/chardev/char-fd.c
> index 2c9b2ce567..bf9803b4c9 100644
> --- a/chardev/char-fd.c
> +++ b/chardev/char-fd.c
> @@ -59,7 +59,10 @@ static gboolean fd_chr_read(QIOChannel *chan,
> GIOCondition cond, void *opaque)
> chan, (gchar *)buf, len, NULL);
> if (ret == 0) {
> remove_fd_in_watch(chr);
> -qemu_chr_be_event(chr, CHR_EVENT_CLOSED);
> +if (!qio_channel_file_is_opened(s->ioc_out)) {
> +/* only send close event if both in & out channels are closed */
> +qemu_chr_be_event(chr, CHR_EVENT_CLOSED);
... here what
Re: [Qemu-devel] [PATCH 1/4] spapr: remove irq_hint parameter from spapr_irq_alloc()
On Mon, May 28, 2018 at 09:06:12AM +0200, Cédric Le Goater wrote: > On 05/28/2018 08:17 AM, Thomas Huth wrote: > > On 25.05.2018 16:02, Greg Kurz wrote: > >> On Fri, 18 May 2018 18:44:02 +0200 > >> Cédric Le Goater wrote: > >> > >>> This IRQ number hint can possibly be used by the VIO devices if the > >>> "irq" property is defined on the command line but it seems it is never > >>> the case. It is not used in libvirt for instance. So, let's remove it > >>> to simplify future changes. > >>> > >> > >> Setting an irq manually looks a bit anachronistic. I doubt anyone would > >> do that nowadays, and the patch does a nice cleanup. So this looks like > >> a good idea. > > [...] > >>> diff --git a/hw/ppc/spapr_vio.c b/hw/ppc/spapr_vio.c > >>> index 472dd6f33a96..cc064f64fccf 100644 > >>> --- a/hw/ppc/spapr_vio.c > >>> +++ b/hw/ppc/spapr_vio.c > >>> @@ -455,7 +455,7 @@ static void spapr_vio_busdev_realize(DeviceState > >>> *qdev, Error **errp) > >>> dev->qdev.id = id; > >>> } > >>> > >>> -dev->irq = spapr_irq_alloc(spapr, dev->irq, false, _err); > >>> +dev->irq = spapr_irq_alloc(spapr, false, _err); > >> > >> Silently breaking "irq" like this looks wrong. I'd rather officially remove > >> it first (ie, kill spapr_vio_props, -5 lines in spapr_vio.c). > >> > >> Of course, this raises the question of interface deprecation, and it should > >> theoretically follow the process described at: > >> > >> https://wiki.qemu.org/Features/LegacyRemoval#Rules_for_removing_an_interface > >> > >> Cc'ing Thomas, our Chief Deprecation Officer, for insights :) > > > > The property is a public interface. Just because it's not used by > > libvirt does not mean that nobody is using it. So yes, please follow the > > rules and mark it as deprecated first for two release, before you really > > remove it. > > This "irq" property is a problem to introduce a new static layout of IRQ > numbers. It is in complete opposition. > > Can we keep it as it is for old pseries machine (settable) and ignore it > for newer ? Would that be fine ? So, Thomas is right that we need to keep the interface while we go through the deprecation process, even though it's a bit of a pain (like you, I seriously doubt anyone ever used it). But, I think there's a way to avoid that getting in the way of your cleanups too much. A bunch of the current problems are caused because spapr_irq_alloc() conflates two meanings of "allocate": 1) finding a free irq to use for this device and 2) assigning that irq exclusively to this device. I think the first thing to do is to split those two parts. (1) will never take an irq parameter, (2) will always take an irq parameter. To implement the (to be deprecated) "irq" property on vio devices you should skip (1) and just call (2) with the given irq number. The point of this series is to basically get rid of (1), but this first step means we don't need to worry about the hint parameter as we gradually remove it. -- David Gibson| I'll have my music baroque, and my code david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_ | _way_ _around_! http://www.ozlabs.org/~dgibson signature.asc Description: PGP signature
Re: [Qemu-devel] [PATCH v3] target/ppc: Allow privileged access to SPR_PCR
On Mon, Jun 04, 2018 at 06:15:13PM +0930, Joel Stanley wrote:
> The powerpc Linux kernel[1] and skiboot firmware[2] recently gained changes
> that cause the Processor Compatibility Register (PCR) SPR to be cleared.
>
> These changes cause Linux to fail to boot on the Qemu powernv machine
> with an error:
>
> Trying to write privileged spr 338 (0x152) at 30017f0c
>
> With this patch Qemu makes this register available as a hypervisor
> privileged register.
>
> Note that bits set in this register disable features of the processor.
> Currently the only register state that is supported is when the register
> is zeroed (enable all features). This is sufficient for guests to
> once again boot.
>
> [1] https://lkml.kernel.org/r/20180518013742.24095-1-mi...@neuling.org
> [2] https://patchwork.ozlabs.org/patch/915932/
>
> Signed-off-by: Joel Stanley
Applied to ppc-for-3.0, thanks.
> ---
> v2:
> - Change error message to say Invalid instead of Unimplemented
> - Fix compile warning on other powerpc targets, thanks patchew
> v3:
> - Mask against pcr_mask before storing
> - Drop check for non-zero value
> ---
> target/ppc/helper.h | 1 +
> target/ppc/misc_helper.c| 9 +
> target/ppc/translate_init.inc.c | 9 +++--
> 3 files changed, 17 insertions(+), 2 deletions(-)
>
> diff --git a/target/ppc/helper.h b/target/ppc/helper.h
> index 19453c68138a..d751f0e21909 100644
> --- a/target/ppc/helper.h
> +++ b/target/ppc/helper.h
> @@ -17,6 +17,7 @@ DEF_HELPER_2(pminsn, void, env, i32)
> DEF_HELPER_1(rfid, void, env)
> DEF_HELPER_1(hrfid, void, env)
> DEF_HELPER_2(store_lpcr, void, env, tl)
> +DEF_HELPER_2(store_pcr, void, env, tl)
> #endif
> DEF_HELPER_1(check_tlb_flush_local, void, env)
> DEF_HELPER_1(check_tlb_flush_global, void, env)
> diff --git a/target/ppc/misc_helper.c b/target/ppc/misc_helper.c
> index 8c8cba5cc6f1..b88493009609 100644
> --- a/target/ppc/misc_helper.c
> +++ b/target/ppc/misc_helper.c
> @@ -20,6 +20,7 @@
> #include "cpu.h"
> #include "exec/exec-all.h"
> #include "exec/helper-proto.h"
> +#include "qemu/error-report.h"
>
> #include "helper_regs.h"
>
> @@ -98,6 +99,14 @@ void helper_store_ptcr(CPUPPCState *env, target_ulong val)
> tlb_flush(CPU(cpu));
> }
> }
> +
> +void helper_store_pcr(CPUPPCState *env, target_ulong value)
> +{
> +PowerPCCPU *cpu = ppc_env_get_cpu(env);
> +PowerPCCPUClass *pcc = POWERPC_CPU_GET_CLASS(cpu);
> +
> +env->spr[SPR_PCR] = value & pcc->pcr_mask;
> +}
> #endif /* defined(TARGET_PPC64) */
>
> void helper_store_pidr(CPUPPCState *env, target_ulong val)
> diff --git a/target/ppc/translate_init.inc.c b/target/ppc/translate_init.inc.c
> index ab782cb32aaa..1a89017ddea8 100644
> --- a/target/ppc/translate_init.inc.c
> +++ b/target/ppc/translate_init.inc.c
> @@ -424,6 +424,10 @@ static void spr_write_ptcr(DisasContext *ctx, int sprn,
> int gprn)
> gen_helper_store_ptcr(cpu_env, cpu_gpr[gprn]);
> }
>
> +static void spr_write_pcr(DisasContext *ctx, int sprn, int gprn)
> +{
> +gen_helper_store_pcr(cpu_env, cpu_gpr[gprn]);
> +}
> #endif
> #endif
>
> @@ -7957,11 +7961,12 @@ static void gen_spr_power6_common(CPUPPCState *env)
> #endif
> /*
> * Register PCR to report POWERPC_EXCP_PRIV_REG instead of
> - * POWERPC_EXCP_INVAL_SPR.
> + * POWERPC_EXCP_INVAL_SPR in userspace. Permit hypervisor access.
> */
> -spr_register(env, SPR_PCR, "PCR",
> +spr_register_hv(env, SPR_PCR, "PCR",
> SPR_NOACCESS, SPR_NOACCESS,
> SPR_NOACCESS, SPR_NOACCESS,
> + _read_generic, _write_pcr,
> 0x);
> }
>
--
David Gibson| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson
signature.asc
Description: PGP signature
Re: [Qemu-devel] [PATCH v11 4/5] i386: Enable TOPOEXT feature on AMD EPYC CPU
On Thu, May 24, 2018 at 11:43:33AM -0400, Babu Moger wrote:
> Enable TOPOEXT feature on EPYC CPU. This is required to support
> hyperthreading on VM guests. Also extend xlevel to 0x801E.
>
> Disable TOPOEXT feature for legacy machines and also disable
> TOPOEXT feature if the config cannot be supported.
>
> Signed-off-by: Babu Moger
> ---
> include/hw/i386/pc.h | 4
> target/i386/cpu.c| 37 +++--
> 2 files changed, 39 insertions(+), 2 deletions(-)
>
> diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
> index a0c269f..9c8db3d 100644
> --- a/include/hw/i386/pc.h
> +++ b/include/hw/i386/pc.h
> @@ -302,6 +302,10 @@ bool e820_get_entry(int, uint32_t, uint64_t *, uint64_t
> *);
> .driver = TYPE_X86_CPU,\
> .property = "legacy-cache",\
> .value= "on",\
> +},{\
> +.driver = "EPYC-" TYPE_X86_CPU,\
> +.property = "topoext",\
> +.value= "off",\
> },
>
> #define PC_COMPAT_2_11 \
> diff --git a/target/i386/cpu.c b/target/i386/cpu.c
> index 9f8bad9..0b62356 100644
> --- a/target/i386/cpu.c
> +++ b/target/i386/cpu.c
> @@ -486,6 +486,20 @@ static void encode_topo_cpuid801e(CPUState *cs,
> X86CPU *cpu,
> }
>
> /*
> + * Check if we can support this topology
> + * Fail if number of cores are beyond the supported config
> + * or nr_threads is more than 2
> + */
> +static int verify_topology(int nr_cores, int nr_threads)
I suggest using a more descriptive name, like
"topology_supports_topoext()".
> +{
> +if ((nr_cores > (MAX_CORES_IN_NODE * MAX_NODES_PER_SOCKET)) ||
> +(nr_threads > 2)) {
> +return 0;
Oh, this is what I was looking for, on the previous patches. I
suggest moving this to a separate patch, separate from the patch
that changes the EPYC CPU model.
> +}
> +return 1;
> +}
> +
> +/*
> * Definitions of the hardcoded cache entries we expose:
> * These are legacy cache values. If there is a need to change any
> * of these values please use builtin_x86_defs
> @@ -2531,7 +2545,8 @@ static X86CPUDefinition builtin_x86_defs[] = {
> .features[FEAT_8000_0001_ECX] =
> CPUID_EXT3_OSVW | CPUID_EXT3_3DNOWPREFETCH |
> CPUID_EXT3_MISALIGNSSE | CPUID_EXT3_SSE4A | CPUID_EXT3_ABM |
> -CPUID_EXT3_CR8LEG | CPUID_EXT3_SVM | CPUID_EXT3_LAHF_LM,
> +CPUID_EXT3_CR8LEG | CPUID_EXT3_SVM | CPUID_EXT3_LAHF_LM |
> +CPUID_EXT3_TOPOEXT,
> .features[FEAT_7_0_EBX] =
> CPUID_7_0_EBX_FSGSBASE | CPUID_7_0_EBX_BMI1 | CPUID_7_0_EBX_AVX2
> |
> CPUID_7_0_EBX_SMEP | CPUID_7_0_EBX_BMI2 | CPUID_7_0_EBX_RDSEED |
> @@ -2576,7 +2591,8 @@ static X86CPUDefinition builtin_x86_defs[] = {
> .features[FEAT_8000_0001_ECX] =
> CPUID_EXT3_OSVW | CPUID_EXT3_3DNOWPREFETCH |
> CPUID_EXT3_MISALIGNSSE | CPUID_EXT3_SSE4A | CPUID_EXT3_ABM |
> -CPUID_EXT3_CR8LEG | CPUID_EXT3_SVM | CPUID_EXT3_LAHF_LM,
> +CPUID_EXT3_CR8LEG | CPUID_EXT3_SVM | CPUID_EXT3_LAHF_LM |
> +CPUID_EXT3_TOPOEXT,
> .features[FEAT_8000_0008_EBX] =
> CPUID_8000_0008_EBX_IBPB,
> .features[FEAT_7_0_EBX] =
> @@ -4156,6 +4172,12 @@ void cpu_x86_cpuid(CPUX86State *env, uint32_t index,
> uint32_t count,
> break;
> case 0x801D:
> *eax = 0;
> +/* Check if we can support this topology */
> +if (!verify_topology(cs->nr_cores, cs->nr_threads)) {
> +/* Disable topology extention */
> +env->features[FEAT_8000_0001_ECX] &= !CPUID_EXT3_TOPOEXT;
> +break;
Please don't change CPUX86State inside cpu_x86_cpuid(). This
belongs to x86_cpu_realizefn().
Also, we want this:
"-cpu EPYC -smp cores=64"
to not error out, because it's a supported configuration today.
But this:
"-cpu EPYC,+topoext -smp cores=64"
should error out because the user explicitly requested an
unsupported configuration.
You can implement this by checking if TOPOEXT is set on
env->user_features.
> +}
> switch (count) {
> case 0: /* L1 dcache info */
> encode_cache_cpuid801d(env->cache_info_amd.l1d_cache, cs,
> @@ -4180,6 +4202,12 @@ void cpu_x86_cpuid(CPUX86State *env, uint32_t index,
> uint32_t count,
> break;
> case 0x801E:
> assert(cpu->core_id <= 255);
> +/* Check if we can support this topology */
> +if (!verify_topology(cs->nr_cores, cs->nr_threads)) {
> +/* Disable topology extention */
> +env->features[FEAT_8000_0001_ECX] &= !CPUID_EXT3_TOPOEXT;
> +break;
> +}
> encode_topo_cpuid801e(cs, cpu,
>eax, ebx, ecx, edx);
> break;
> @@ -4644,6 +4672,11 @@ static void x86_cpu_expand_features(X86CPU *cpu, Error
> **errp)
> x86_cpu_adjust_level(cpu, >cpuid_min_xlevel, 0x800A);
> }
>
Re: [Qemu-devel] [PATCH v1 5/7] docker: Update fedora image to 28
On Mon, 06/04 15:51, Alex Bennée wrote: > From: Fam Zheng > > Signed-off-by: Fam Zheng > Signed-off-by: Alex Bennée > --- > tests/docker/dockerfiles/fedora.docker | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/tests/docker/dockerfiles/fedora.docker > b/tests/docker/dockerfiles/fedora.docker > index b706f42405..65d7761cf5 100644 > --- a/tests/docker/dockerfiles/fedora.docker > +++ b/tests/docker/dockerfiles/fedora.docker > @@ -1,4 +1,4 @@ > -FROM fedora:27 > +FROM fedora:28 > ENV PACKAGES \ > ccache gettext git tar PyYAML sparse flex bison python3 bzip2 hostname \ > gcc gcc-c++ llvm clang make perl which bc findutils glib2-devel \ > -- > 2.17.0 > This one is in master now. Fam
Re: [Qemu-devel] [PATCH v11 3/5] i386: Add support for CPUID_8000_001E for AMD
On Thu, May 24, 2018 at 11:43:32AM -0400, Babu Moger wrote:
> Add support for cpuid leaf CPUID_8000_001E. Build the config that closely
> match the underlying hardware. Please refer to the Processor Programming
> Reference (PPR) for AMD Family 17h Model for more details.
>
> Signed-off-by: Babu Moger
> ---
> target/i386/cpu.c | 61
> +++
> 1 file changed, 61 insertions(+)
>
> diff --git a/target/i386/cpu.c b/target/i386/cpu.c
> index 0d423e5..9f8bad9 100644
> --- a/target/i386/cpu.c
> +++ b/target/i386/cpu.c
> @@ -429,6 +429,62 @@ static void encode_cache_cpuid801d(CPUCacheInfo
> *cache, CPUState *cs,
> (cache->complex_indexing ? CACHE_COMPLEX_IDX : 0);
> }
>
> +/* Data structure to hold the configuration info for a given core index */
> +struct core_topology {
> +/* core complex id of the current core index */
> +int ccx_id;
> +/* Adjusted core id for this core index in the topology */
What's an "adjusted core id"?
> +int core_id;
> +/* Node id for this core index */
> +int node_id;
> +/* Number of nodes in this config, 0 based */
> +int num_nodes;
I suggest making num_nodes carry the actual number of nodes, and
using (num_nodes - 1) when building CPUID data.
> +};
> +
> +/*
> + * Build the configuration closely match the EPYC hardware. Using the EPYC
> + * hardware configuration values (MAX_CCX, MAX_CORES_IN_CCX,
> MAX_CORES_IN_NODE)
> + * right now. This could change in future.
> + * nr_cores : Total number of cores in the config
> + * core_id : Core index of the current CPU
> + * topo : Data structure to hold all the config info for this core index
> + */
> +static void build_core_topology(int nr_cores, int core_id,
> +struct core_topology *topo)
> +{
> +int nodes, cores_in_ccx;
> +
> +/* First get the number of nodes required */
> +nodes = nodes_in_socket(nr_cores);
> +
> +cores_in_ccx = cores_in_core_complex(nr_cores);
> +
> +topo->node_id = core_id / (cores_in_ccx * MAX_CCX);
> +topo->ccx_id = (core_id % (cores_in_ccx * MAX_CCX)) / cores_in_ccx;
> +topo->core_id = core_id % cores_in_ccx;
> +/* num_nodes is 0 based, return n - 1 */
> +topo->num_nodes = nodes - 1;
> +}
Is this guaranteed to work with any nr_cores value, or only with
a few specific values that match real hardware?
> +
> +/* Encode cache info for CPUID[801E] */
> +static void encode_topo_cpuid801e(CPUState *cs, X86CPU *cpu,
> + uint32_t *eax, uint32_t *ebx,
> + uint32_t *ecx, uint32_t *edx)
> +{
> +struct core_topology topo = {0};
> +
> +build_core_topology(cs->nr_cores, cpu->core_id, );
> +*eax = cpu->apic_id;
> +if (cs->nr_threads - 1) {
> +*ebx = ((cs->nr_threads - 1) << 8) | (topo.node_id << 3) |
> +(topo.ccx_id << 2) | topo.core_id;
> +} else {
> +*ebx = (topo.node_id << 4) | (topo.ccx_id << 3) | topo.core_id;
> +}
This part confuses me a lot. Where are those bit offsets in EBX
defined?
Is this guaranteed to work with any cs->nr_cores value?
> +*ecx = (topo.num_nodes << 8) | (cpu->socket_id << 2) | topo.node_id;
Like on EBX, I'm confused by the bit offsets. Where are they
defined?
Probably it's safer to not allow TOPOEXT to be enabled if the CPU
socket/core/thread topology configuration can't generate a sane
node/core-complex topology.
> +*edx = 0;
> +}
> +
> /*
> * Definitions of the hardcoded cache entries we expose:
> * These are legacy cache values. If there is a need to change any
> @@ -4122,6 +4178,11 @@ void cpu_x86_cpuid(CPUX86State *env, uint32_t index,
> uint32_t count,
> break;
> }
> break;
> +case 0x801E:
> +assert(cpu->core_id <= 255);
> +encode_topo_cpuid801e(cs, cpu,
> + eax, ebx, ecx, edx);
> +break;
> case 0xC000:
> *eax = env->cpuid_xlevel2;
> *ebx = 0;
> --
> 1.8.3.1
>
>
--
Eduardo
Re: [Qemu-devel] [PATCH] qemu: Introduce VIRTIO_NET_F_STANDBY feature bit to virtio_net
On 2018年06月05日 09:41, Samudrala, Sridhar wrote:
Ping on this patch now that the kernel patches are accepted into
davem's net-next tree.
https://patchwork.ozlabs.org/cover/920005/
On 5/7/2018 4:09 PM, Sridhar Samudrala wrote:
This feature bit can be used by hypervisor to indicate virtio_net
device to
act as a standby for another device with the same MAC address.
I tested this with a small change to the patch to mark the STANDBY
feature 'true'
by default as i am using libvirt to start the VMs.
Is there a way to pass the newly added feature bit 'standby' to qemu
via libvirt
XML file?
Maybe you can try qemu command line passthrough:
https://libvirt.org/drvqemu.html#qemucommand
The patch looks good to me. Let's see if Michael have any comment.
Thanks
Signed-off-by: Sridhar Samudrala
---
hw/net/virtio-net.c | 2 ++
include/standard-headers/linux/virtio_net.h | 3 +++
2 files changed, 5 insertions(+)
diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
index 90502fca7c..38b3140670 100644
--- a/hw/net/virtio-net.c
+++ b/hw/net/virtio-net.c
@@ -2198,6 +2198,8 @@ static Property virtio_net_properties[] = {
true),
DEFINE_PROP_INT32("speed", VirtIONet, net_conf.speed,
SPEED_UNKNOWN),
DEFINE_PROP_STRING("duplex", VirtIONet, net_conf.duplex_str),
+ DEFINE_PROP_BIT64("standby", VirtIONet, host_features,
VIRTIO_NET_F_STANDBY,
+ false),
DEFINE_PROP_END_OF_LIST(),
};
diff --git a/include/standard-headers/linux/virtio_net.h
b/include/standard-headers/linux/virtio_net.h
index e9f255ea3f..01ec09684c 100644
--- a/include/standard-headers/linux/virtio_net.h
+++ b/include/standard-headers/linux/virtio_net.h
@@ -57,6 +57,9 @@
* Steering */
#define VIRTIO_NET_F_CTRL_MAC_ADDR 23 /* Set MAC address */
+#define VIRTIO_NET_F_STANDBY 62 /* Act as standby for
another device
+ * with the same MAC.
+ */
#define VIRTIO_NET_F_SPEED_DUPLEX 63 /* Device set linkspeed and
duplex */
#ifndef VIRTIO_NET_NO_LEGACY
Re: [Qemu-devel] [PATCH v11 2/5] i386: Populate AMD Processor Cache Information for cpuid 0x8000001D
On Thu, May 24, 2018 at 11:43:31AM -0400, Babu Moger wrote:
> Add information for cpuid 0x801D leaf. Populate cache topology information
> for different cache types (Data Cache, Instruction Cache, L2 and L3) supported
> by 0x801D leaf. Please refer to the Processor Programming Reference (PPR)
> for AMD Family 17h Model for more details.
>
> Signed-off-by: Babu Moger
> ---
> target/i386/cpu.c | 117
> ++
> target/i386/kvm.c | 29 --
> 2 files changed, 143 insertions(+), 3 deletions(-)
>
> diff --git a/target/i386/cpu.c b/target/i386/cpu.c
> index 5c9bdc9..0d423e5 100644
> --- a/target/i386/cpu.c
> +++ b/target/i386/cpu.c
> @@ -337,6 +337,99 @@ static void encode_cache_cpuid8006(CPUCacheInfo *l2,
> }
>
> /*
> + * Definitions used for building CPUID Leaf 0x801D and 0x801E
> + * Please refer to the AMD64 Architecture Programmer’s Manual Volume 3.
> + * Define the constants to build the cpu topology. Right now, TOPOEXT
> + * feature is enabled only on EPYC. So, these constants are based on
> + * EPYC supported configurations. We may need to handle the cases if
> + * these values change in future.
> + */
> +/* Maximum core complexes in a node */
> +#define MAX_CCX 2
> +/* Maximum cores in a core complex */
> +#define MAX_CORES_IN_CCX 4
> +/* Maximum cores in a node */
> +#define MAX_CORES_IN_NODE 8
> +/* Maximum nodes in a socket */
> +#define MAX_NODES_PER_SOCKET 4
> +
> +/*
> + * Figure out the number of nodes required to build this config.
> + * Max cores in a node is 4
> + */
> +static int nodes_in_socket(int nr_cores)
> +{
> +int nodes;
> +
> +nodes = DIV_ROUND_UP(nr_cores, MAX_CORES_IN_NODE);
> +
> + /* Hardware does not support config with 3 nodes, return 4 in that case */
> +return (nodes == 3) ? 4 : nodes;
> +}
> +
> +/*
> + * Decide the number of cores in a core complex with the given nr_cores using
> + * following set constants MAX_CCX, MAX_CORES_IN_CCX, MAX_CORES_IN_NODE and
> + * MAX_NODES_PER_SOCKET. Maintain symmetry as much as possible
> + * L3 cache is shared across all cores in a core complex. So, this will also
> + * tell us how many cores are sharing the L3 cache.
> + */
> +static int cores_in_core_complex(int nr_cores)
> +{
> +int nodes;
> +
> +/* Check if we can fit all the cores in one core complex */
> +if (nr_cores <= MAX_CORES_IN_CCX) {
> +return nr_cores;
> +}
> +/* Get the number of nodes required to build this config */
> +nodes = nodes_in_socket(nr_cores);
> +
> +/*
> + * Divide the cores accros all the core complexes
> + * Return rounded up value
> + */
> +return DIV_ROUND_UP(nr_cores, nodes * MAX_CCX);
> +}
It's nice that we try to figure out a good default even on weird
topologies:
Reviewed-by: Eduardo Habkost
But I still worry about the complexity of compat code in the
future if we decide to change the results of this function a
little bit. Maybe we should allow TOPOEXT to be enabled only on
cases where we really know how to fit the cores in a round number
of nodes/core-complexes?
Let's do this in a follow-up patch?
--
Eduardo
Re: [Qemu-devel] [PATCH] qemu: Introduce VIRTIO_NET_F_STANDBY feature bit to virtio_net
Ping on this patch now that the kernel patches are accepted into davem's
net-next tree.
https://patchwork.ozlabs.org/cover/920005/
On 5/7/2018 4:09 PM, Sridhar Samudrala wrote:
This feature bit can be used by hypervisor to indicate virtio_net device to
act as a standby for another device with the same MAC address.
I tested this with a small change to the patch to mark the STANDBY feature
'true'
by default as i am using libvirt to start the VMs.
Is there a way to pass the newly added feature bit 'standby' to qemu via libvirt
XML file?
Signed-off-by: Sridhar Samudrala
---
hw/net/virtio-net.c | 2 ++
include/standard-headers/linux/virtio_net.h | 3 +++
2 files changed, 5 insertions(+)
diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
index 90502fca7c..38b3140670 100644
--- a/hw/net/virtio-net.c
+++ b/hw/net/virtio-net.c
@@ -2198,6 +2198,8 @@ static Property virtio_net_properties[] = {
true),
DEFINE_PROP_INT32("speed", VirtIONet, net_conf.speed, SPEED_UNKNOWN),
DEFINE_PROP_STRING("duplex", VirtIONet, net_conf.duplex_str),
+DEFINE_PROP_BIT64("standby", VirtIONet, host_features,
VIRTIO_NET_F_STANDBY,
+ false),
DEFINE_PROP_END_OF_LIST(),
};
diff --git a/include/standard-headers/linux/virtio_net.h b/include/standard-headers/linux/virtio_net.h
index e9f255ea3f..01ec09684c 100644
--- a/include/standard-headers/linux/virtio_net.h
+++ b/include/standard-headers/linux/virtio_net.h
@@ -57,6 +57,9 @@
* Steering */
#define VIRTIO_NET_F_CTRL_MAC_ADDR 23 /* Set MAC address */
+#define VIRTIO_NET_F_STANDBY 62/* Act as standby for another device
+ * with the same MAC.
+ */
#define VIRTIO_NET_F_SPEED_DUPLEX 63 /* Device set linkspeed and duplex */
#ifndef VIRTIO_NET_NO_LEGACY
Re: [Qemu-devel] [PATCH v4 08/14] spapr: handle pc-dimm unplug via hotplug handler chain
On Thu, May 17, 2018 at 10:15:21AM +0200, David Hildenbrand wrote:
> Let's handle it via hotplug_handler_unplug(). E.g. necessary to hotplug/
> unplug memory devices (which a pc-dimm is) later.
>
> Signed-off-by: David Hildenbrand
> ---
> hw/ppc/spapr.c | 23 +++
> 1 file changed, 19 insertions(+), 4 deletions(-)
>
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index 2f315f963b..286c38c842 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -3291,7 +3291,8 @@ static sPAPRDIMMState
> *spapr_recover_pending_dimm_state(sPAPRMachineState *ms,
> /* Callback to be called during DRC release. */
> void spapr_lmb_release(DeviceState *dev)
> {
> -sPAPRMachineState *spapr = SPAPR_MACHINE(qdev_get_hotplug_handler(dev));
> +HotplugHandler *hotplug_ctrl = qdev_get_hotplug_handler(dev);
> +sPAPRMachineState *spapr = SPAPR_MACHINE(hotplug_ctrl);
> sPAPRDIMMState *ds = spapr_pending_dimm_unplugs_find(spapr,
> PC_DIMM(dev));
>
> /* This information will get lost if a migration occurs
> @@ -3309,9 +3310,21 @@ void spapr_lmb_release(DeviceState *dev)
>
> /*
> * Now that all the LMBs have been removed by the guest, call the
> - * pc-dimm unplug handler to cleanup up the pc-dimm device.
> + * unplug handler chain. This can never fail.
> */
> -pc_dimm_memory_unplug(dev, MACHINE(spapr));
> +hotplug_ctrl = qdev_get_hotplug_handler(dev);
You're double initializing hotplug_ctrl to the same thing here, AFAICT.
> +hotplug_handler_unplug(hotplug_ctrl, dev, _abort);
> +}
> +
> +static void spapr_memory_unplug(HotplugHandler *hotplug_dev, DeviceState
> *dev,
> +Error **errp)
> +{
> +sPAPRMachineState *spapr = SPAPR_MACHINE(hotplug_dev);
> +sPAPRDIMMState *ds = spapr_pending_dimm_unplugs_find(spapr,
> PC_DIMM(dev));
> +
> +g_assert(ds);
> +g_assert(!ds->nr_lmbs);
> +pc_dimm_memory_unplug(dev, MACHINE(hotplug_dev));
> object_unparent(OBJECT(dev));
> spapr_pending_dimm_unplugs_remove(spapr, ds);
> }
> @@ -3608,7 +3621,9 @@ static void spapr_machine_device_unplug(HotplugHandler
> *hotplug_dev,
> Error *local_err = NULL;
>
> /* final stage hotplug handler */
> -if (dev->parent_bus && dev->parent_bus->hotplug_handler) {
> +if (object_dynamic_cast(OBJECT(dev), TYPE_PC_DIMM)) {
> +spapr_memory_unplug(hotplug_dev, dev, _err);
> +} else if (dev->parent_bus && dev->parent_bus->hotplug_handler) {
> hotplug_handler_unplug(dev->parent_bus->hotplug_handler, dev,
> _err);
> }
--
David Gibson| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson
signature.asc
Description: PGP signature
Re: [Qemu-devel] [PATCH v4 09/14] spapr: handle cpu core unplug via hotplug handler chain
On Thu, May 17, 2018 at 10:15:22AM +0200, David Hildenbrand wrote:
> Let's handle it via hotplug_handler_unplug().
>
> Signed-off-by: David Hildenbrand
Acked-by: David Gibson
> ---
> hw/ppc/spapr.c | 13 -
> 1 file changed, 12 insertions(+), 1 deletion(-)
>
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index 286c38c842..13d153b5a6 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -3412,7 +3412,16 @@ static void *spapr_populate_hotplug_cpu_dt(CPUState
> *cs, int *fdt_offset,
> /* Callback to be called during DRC release. */
> void spapr_core_release(DeviceState *dev)
> {
> -MachineState *ms = MACHINE(qdev_get_hotplug_handler(dev));
> +HotplugHandler *hotplug_ctrl = qdev_get_hotplug_handler(dev);
> +
> +/* Call the unplug handler chain. This can never fail. */
> +hotplug_handler_unplug(hotplug_ctrl, dev, _abort);
> +}
> +
> +static void spapr_core_unplug(HotplugHandler *hotplug_dev, DeviceState *dev,
> + Error **errp)
> +{
> +MachineState *ms = MACHINE(hotplug_dev);
> sPAPRMachineClass *smc = SPAPR_MACHINE_GET_CLASS(ms);
> CPUCore *cc = CPU_CORE(dev);
> CPUArchId *core_slot = spapr_find_cpu_slot(ms, cc->core_id, NULL);
> @@ -3623,6 +3632,8 @@ static void spapr_machine_device_unplug(HotplugHandler
> *hotplug_dev,
> /* final stage hotplug handler */
> if (object_dynamic_cast(OBJECT(dev), TYPE_PC_DIMM)) {
> spapr_memory_unplug(hotplug_dev, dev, _err);
> +} else if (object_dynamic_cast(OBJECT(dev), TYPE_SPAPR_CPU_CORE)) {
> +spapr_core_unplug(hotplug_dev, dev, _err);
> } else if (dev->parent_bus && dev->parent_bus->hotplug_handler) {
> hotplug_handler_unplug(dev->parent_bus->hotplug_handler, dev,
> _err);
--
David Gibson| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson
signature.asc
Description: PGP signature
Re: [Qemu-devel] [PATCH] CODING_STYLE: Define our preferred form for multiline comments
On Mon, 4 Jun 2018 17:21:40 +0100 Peter Maydell wrote: > The codebase has a bit of a mix of > /* multiline comments > * like this > */ > and > /* multiline comments like this > in the GNU Coding Standards style */ > > State a preference for the former. > > Signed-off-by: Peter Maydell > --- > I admit that to some extent I'm imposing my aesthetic > preferences here; pretty sure we have a lot more style > 1 comments than style 2, though. > --- > CODING_STYLE | 13 + > 1 file changed, 13 insertions(+) > > diff --git a/CODING_STYLE b/CODING_STYLE > index 12ba58ee293..fb1d1f1cd62 100644 > --- a/CODING_STYLE > +++ b/CODING_STYLE > @@ -124,6 +124,19 @@ We use traditional C-style /* */ comments and avoid // > comments. > Rationale: The // form is valid in C99, so this is purely a matter of > consistency of style. The checkpatch script will warn you about this. > > +Multiline comments blocks should have a row of stars on the left > +and the terminating */ on its own line: > +/* like > + * this > + */ > +Putting the initial /* on its own line is accepted, but not required. Could we say "at maintainer discretion", or is that always implied? The asymmetry of the proposed standard is not my favorite and a mostly blank line before and after further supports standing out from surrounding code. Note that the kernel coding style, except for certain exceptions, is: /* * This is a * multi-line * comment */ Thanks, Alex > +(Some of the existing comments in the codebase use the GNU Coding > +Standards form which does not have stars on the left; avoid this > +when writing new comments.) > + > +Rationale: Consistency, and ease of visually picking out a multiline > +comment from the surrounding code. > + > 8. trace-events style > > 8.1 0x prefix
Re: [Qemu-devel] [PATCH v4 07/14] spapr: route all memory devices through the machine hotplug handler
On Thu, May 17, 2018 at 10:15:20AM +0200, David Hildenbrand wrote:
> Necessary to hotplug them cleanly later.
>
> Signed-off-by: David Hildenbrand
As for PC, I think it would be nicer to drop the explicit check
against PC_DIMM, since it is covered by MEMORY_DEVICE.
> ---
> hw/ppc/spapr.c | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index b7c5c95f7a..2f315f963b 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -3666,6 +3666,7 @@ static HotplugHandler
> *spapr_get_hotplug_handler(MachineState *machine,
> DeviceState *dev)
> {
> if (object_dynamic_cast(OBJECT(dev), TYPE_PC_DIMM) ||
> +object_dynamic_cast(OBJECT(dev), TYPE_MEMORY_DEVICE) ||
> object_dynamic_cast(OBJECT(dev), TYPE_SPAPR_CPU_CORE)) {
> return HOTPLUG_HANDLER(machine);
> }
--
David Gibson| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson
signature.asc
Description: PGP signature
Re: [Qemu-devel] [PATCH v4 06/14] spapr: prepare for multi stage hotplug handlers
On Thu, May 17, 2018 at 10:15:19AM +0200, David Hildenbrand wrote:
> For multi stage hotplug handlers, we'll have to do some error handling
> in some hotplug functions, so let's use a local error variable (except
> for unplug requests).
>
> Also, add code to pass control to the final stage hotplug handler at the
> parent bus.
>
> Signed-off-by: David Hildenbrand
> ---
> hw/ppc/spapr.c | 54 +++---
> 1 file changed, 43 insertions(+), 11 deletions(-)
>
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index ebf30dd60b..b7c5c95f7a 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -3571,27 +3571,48 @@ static void spapr_machine_device_plug(HotplugHandler
> *hotplug_dev,
> {
> MachineState *ms = MACHINE(hotplug_dev);
> sPAPRMachineClass *smc = SPAPR_MACHINE_GET_CLASS(ms);
> +Error *local_err = NULL;
>
> +/* final stage hotplug handler */
> if (object_dynamic_cast(OBJECT(dev), TYPE_PC_DIMM)) {
> int node;
>
> if (!smc->dr_lmb_enabled) {
> -error_setg(errp, "Memory hotplug not supported for this
> machine");
> -return;
> +error_setg(_err,
> + "Memory hotplug not supported for this machine");
> +goto out;
> }
> -node = object_property_get_uint(OBJECT(dev), PC_DIMM_NODE_PROP,
> errp);
> -if (*errp) {
> -return;
> +node = object_property_get_uint(OBJECT(dev), PC_DIMM_NODE_PROP,
> +_err);
> +if (local_err) {
> +goto out;
> }
> if (node < 0 || node >= MAX_NODES) {
> -error_setg(errp, "Invaild node %d", node);
> -return;
> +error_setg(_err, "Invaild node %d", node);
> +goto out;
> }
>
> -spapr_memory_plug(hotplug_dev, dev, node, errp);
> +spapr_memory_plug(hotplug_dev, dev, node, _err);
> } else if (object_dynamic_cast(OBJECT(dev), TYPE_SPAPR_CPU_CORE)) {
> -spapr_core_plug(hotplug_dev, dev, errp);
> +spapr_core_plug(hotplug_dev, dev, _err);
> +} else if (dev->parent_bus && dev->parent_bus->hotplug_handler) {
> +hotplug_handler_plug(dev->parent_bus->hotplug_handler, dev,
> _err);
> +}
> +out:
> +error_propagate(errp, local_err);
> +}
> +
> +static void spapr_machine_device_unplug(HotplugHandler *hotplug_dev,
> +DeviceState *dev, Error **errp)
> +{
> +Error *local_err = NULL;
> +
> +/* final stage hotplug handler */
> +if (dev->parent_bus && dev->parent_bus->hotplug_handler) {
As I think Igor said on the equivalent PC patch, I don't quite get
this. Isn't this already handled by the generic hotplug code picking
up the bus's hotplug handler if the machine doesn't supply one?
> +hotplug_handler_unplug(dev->parent_bus->hotplug_handler, dev,
> + _err);
> }
> +error_propagate(errp, local_err);
> }
>
> static void spapr_machine_device_unplug_request(HotplugHandler *hotplug_dev,
> @@ -3618,17 +3639,27 @@ static void
> spapr_machine_device_unplug_request(HotplugHandler *hotplug_dev,
> return;
> }
> spapr_core_unplug_request(hotplug_dev, dev, errp);
> +} else if (dev->parent_bus && dev->parent_bus->hotplug_handler) {
> +hotplug_handler_unplug_request(dev->parent_bus->hotplug_handler, dev,
> + errp);
> }
> }
>
> static void spapr_machine_device_pre_plug(HotplugHandler *hotplug_dev,
>DeviceState *dev, Error **errp)
> {
> +Error *local_err = NULL;
> +
> +/* final stage hotplug handler */
> if (object_dynamic_cast(OBJECT(dev), TYPE_PC_DIMM)) {
> -spapr_memory_pre_plug(hotplug_dev, dev, errp);
> +spapr_memory_pre_plug(hotplug_dev, dev, _err);
> } else if (object_dynamic_cast(OBJECT(dev), TYPE_SPAPR_CPU_CORE)) {
> -spapr_core_pre_plug(hotplug_dev, dev, errp);
> +spapr_core_pre_plug(hotplug_dev, dev, _err);
> +} else if (dev->parent_bus && dev->parent_bus->hotplug_handler) {
> +hotplug_handler_pre_plug(dev->parent_bus->hotplug_handler, dev,
> + _err);
> }
> +error_propagate(errp, local_err);
> }
>
> static HotplugHandler *spapr_get_hotplug_handler(MachineState *machine,
> @@ -3988,6 +4019,7 @@ static void spapr_machine_class_init(ObjectClass *oc,
> void *data)
> mc->get_default_cpu_node_id = spapr_get_default_cpu_node_id;
> mc->possible_cpu_arch_ids = spapr_possible_cpu_arch_ids;
> hc->unplug_request = spapr_machine_device_unplug_request;
> +hc->unplug = spapr_machine_device_unplug;
>
> smc->dr_lmb_enabled = true;
> mc->default_cpu_type = POWERPC_CPU_TYPE_NAME("power8_v2.0");
--
David Gibson| I'll have my music baroque, and my
Re: [Qemu-devel] [PATCH v4 03/14] qdev: let machine hotplug handler to override bus hotplug handler
On Thu, May 17, 2018 at 10:15:16AM +0200, David Hildenbrand wrote:
> From: Igor Mammedov
>
> it will allow to return another hotplug handler than the default
> one for a specific bus based device type. Which is needed to handle
> non trivial plug/unplug sequences that need the access to resources
> configured outside of bus where device is attached.
>
> That will allow for returned hotplug handler to orchestrate wiring
> in arbitrary order, by chaining other hotplug handlers when
> it's needed.
>
> PS:
> It could be used for hybrid virtio-mem and virtio-pmem devices
> where it will return machine as hotplug handler which will do
> necessary wiring at machine level and then pass control down
> the chain to bus specific hotplug handler.
>
> Example of top level hotplug handler override and custom plug sequence:
>
> some_machine_get_hotplug_handler(machine){
> if (object_dynamic_cast(OBJECT(dev), TYPE_SOME_BUS_DEVICE)) {
> return HOTPLUG_HANDLER(machine);
> }
> return NULL;
> }
>
> some_machine_device_plug(hotplug_dev, dev) {
> if (object_dynamic_cast(OBJECT(dev), TYPE_SOME_BUS_DEVICE)) {
> /* do machine specific initialization */
> some_machine_init_special_device(dev)
>
> /* pass control to bus specific handler */
> hotplug_handler_plug(dev->parent_bus->hotplug_handler, dev)
> }
> }
>
> Signed-off-by: Igor Mammedov
> Signed-off-by: David Hildenbrand
Reviewed-by: David Gibson
I've been considering a similar change for a while; we'll also need
something like this in order to do hoplug for PCI devices under p2p
bridges on pseries (there's a PAPR specific hotplug model that we need
to use instead of SHP).
> ---
> hw/core/qdev.c | 6 ++
> include/hw/qdev-core.h | 11 +++
> 2 files changed, 13 insertions(+), 4 deletions(-)
>
> diff --git a/hw/core/qdev.c b/hw/core/qdev.c
> index f6f92473b8..885286f579 100644
> --- a/hw/core/qdev.c
> +++ b/hw/core/qdev.c
> @@ -261,12 +261,10 @@ HotplugHandler
> *qdev_get_machine_hotplug_handler(DeviceState *dev)
>
> HotplugHandler *qdev_get_hotplug_handler(DeviceState *dev)
> {
> -HotplugHandler *hotplug_ctrl;
> +HotplugHandler *hotplug_ctrl = qdev_get_machine_hotplug_handler(dev);
>
> -if (dev->parent_bus && dev->parent_bus->hotplug_handler) {
> +if (hotplug_ctrl == NULL && dev->parent_bus) {
> hotplug_ctrl = dev->parent_bus->hotplug_handler;
> -} else {
> -hotplug_ctrl = qdev_get_machine_hotplug_handler(dev);
> }
> return hotplug_ctrl;
> }
> diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h
> index 9453588160..e6a8eca558 100644
> --- a/include/hw/qdev-core.h
> +++ b/include/hw/qdev-core.h
> @@ -286,6 +286,17 @@ void qdev_init_nofail(DeviceState *dev);
> void qdev_set_legacy_instance_id(DeviceState *dev, int alias_id,
> int required_for_version);
> HotplugHandler *qdev_get_machine_hotplug_handler(DeviceState *dev);
> +/**
> + * qdev_get_hotplug_handler: Get handler responsible for device wiring
> + *
> + * Find HOTPLUG_HANDLER for @dev that provides [pre|un]plug callbacks for it.
> + *
> + * Note: in case @dev has a parent bus, it will be returned as handler unless
> + * machine handler overrides it.
> + *
> + * Returns: pointer to object that implements TYPE_HOTPLUG_HANDLER interface
> + * or NULL if there aren't any.
> + */
> HotplugHandler *qdev_get_hotplug_handler(DeviceState *dev);
> void qdev_unplug(DeviceState *dev, Error **errp);
> void qdev_simple_device_unplug_cb(HotplugHandler *hotplug_dev,
--
David Gibson| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson
signature.asc
Description: PGP signature
Re: [Qemu-devel] [PATCH v2 2/2] vl: fix use of --daemonize with --preconfig
On Mon, Jun 04, 2018 at 05:01:11PM +0200, Igor Mammedov wrote:
[...]
> I'd prefer your patch, as it doesn't break error handling,
> or maybe even shorter version which keeps state transitions in one place:
>
> diff --git a/vl.c b/vl.c
> index c4fe255..fa44138 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -1956,7 +1956,7 @@ static void main_loop(void)
> #ifdef CONFIG_PROFILER
> int64_t ti;
> #endif
> -do {
> +while (!main_loop_should_exit()) {
> #ifdef CONFIG_PROFILER
> ti = profile_getclock();
> #endif
> @@ -1964,7 +1964,7 @@ static void main_loop(void)
> #ifdef CONFIG_PROFILER
> dev_time += profile_getclock() - ti;
> #endif
> -} while (!main_loop_should_exit());
> +}
> }
Would this also fix the bug mentioned at:
"vl.c: make default main_loop_wait() timeout independed of slirp"?
--
Eduardo
Re: [Qemu-devel] [PATCH] vl.c: make default main_loop_wait() timeout independed of slirp
On Mon, Jun 04, 2018 at 10:14:38PM +0200, Igor Mammedov wrote: > Since commit (047f7038f58 cli: add --preconfig option) QEMU is > stuck with indefinite timeout in os_host_main_loop_wait() > at RUN_STATE_PRECONFIG even if --preconfig option wasn't used > when it's started with -nodefaults CLI option like this: > > ./s390x-softmmu/qemu-system-s390x -nodefaults > > It's caused by the fact that slirp_pollfds_fill() bails out early > and slirp_update_timeout() won't be called to update timeout > to a reasonable value (1 sec) so timeout would be left with > infinite value (0x). > > Default infinite timeout though doen't make sense and reducing > it to 1 second as in slirp_update_timeout() won't affect host. I don't get this part. Why default infinite timeout doesn't make sense? Why would a 1 second timeout make sense? > Fix issue by simplifying default timeout to the same 1sec as it > is in slirp_update_timeout() and cleanup the later. It makes > default timeout the same regardless of slirp_pollfds_fill() > exited early or not (i.e. -nodefaults won't have any effect on > main_loop_wait() anymore, which would provide more consistent > behavior between both variants of startup). > > Reported-by: Lukáš Doktor > Signed-off-by: Igor Mammedov > --- > PS: > it doesn't fix issue reported by Max where > "echo foo | $QEMU" > is also broken due to commit 047f7038f58, but there is antoher fix > on the list to fix that (either Michal's or Daniel's). [...] -- Eduardo
Re: [Qemu-devel] [PATCH v2 1/2] vl: don't use RUN_STATE_PRECONFIG as initial state
On Mon, Jun 04, 2018 at 04:21:47PM +0200, Michal Privoznik wrote:
[...]
> > @@ -3572,7 +3570,12 @@ int main(int argc, char **argv, char **envp)
> > }
> > break;
> > case QEMU_OPTION_preconfig:
> > -preconfig_exit_requested = false;
> > +if (!runstate_check(RUN_STATE_NONE)) {
> > +error_report("'--preconfig' and '--incoming' options
> > are "
> > + "mutually exclusive");
> > +exit(EXIT_FAILURE);
> > +}
> > +runstate_set(RUN_STATE_PRECONFIG);
>
> Specifying --preconfig twice on the command line now fails with a very
> cryptic message (there's no --incoming).
>
> > break;
> > case QEMU_OPTION_enable_kvm:
> > olist = qemu_find_opts("machine");
> > @@ -3768,9 +3771,12 @@ int main(int argc, char **argv, char **envp)
> > }
> > break;
> > case QEMU_OPTION_incoming:
> > -if (!incoming) {
> > -runstate_set(RUN_STATE_INMIGRATE);
> > +if (!runstate_check(RUN_STATE_NONE)) {
> > +error_report("'--preconfig' and '--incoming' options
> > are "
> > + "mutually exclusive");
> > +exit(EXIT_FAILURE);
> > }
> > +runstate_set(RUN_STATE_INMIGRATE);
>
> Same here. Specifying --incoming twice fails with cryptic message. But
> one can argue that specifying --incoming twice is wrong anyway.
>
Initially I was going to suggest simply not changing runstate
during option parsing to avoid this kind of problem, but maybe
this would be a nice way to implement the command-line parsing
rules:
case QEMU_OPTION_preconfig:
/*
* A INCOMING -> PRECONFIG transition would call:
* error_setg("--preconfig and --incoming options are mutually
exclusive");
*/
try_runstate_set(RUN_STATE_PRECONFIG, _fatal);
break;
case QEMU_OPTION_incoming:
/*
* A PRECONFIG -> INCOMING transition would also call:
* error_setg("--preconfig and --incoming options are mutually
exclusive");
*
* Maybe a INCOMING -> INCOMING transition could
* result in:
* error_setg("--incoming can't be specified twice");
*/
try_runstate_set(RUN_STATE_INMIGRATE, _fatal);
break;
> > incoming = optarg;
> > break;
> > case QEMU_OPTION_only_migratable:
>
> Michal
>
--
Eduardo
Re: [Qemu-devel] [PATCH] cli: Don't run early event loop if no --preconfig was specified
On Mon, Jun 04, 2018 at 11:32:44AM +0100, Daniel P. Berrangé wrote:
[...]
> Avoiding the double-run of main_loop is good, however, I think we should
> also not have put current_run_state in RUN_STATE_PRECONFIG in the first
> place if --preconfig wasn't set. I've sent a patch to fix that problem
> too, so if yours is also applied, it could be changed to just do:
>
> if (current_run_state == RNU_STATE_PRECONFIG) {
> main_loop();
> }
So, this patch is desirable even if we refactor the state machine
as suggested in the other threads, right?
I'm queueing it on machine-next right now.
--
Eduardo
Re: [Qemu-devel] [PATCH v2 1/2] vl: don't use RUN_STATE_PRECONFIG as initial state
On Mon, Jun 04, 2018 at 07:37:15PM +0200, Igor Mammedov wrote:
> On Mon, 4 Jun 2018 17:11:57 +0100
> Daniel P. Berrangé wrote:
>
> > On Mon, Jun 04, 2018 at 05:40:32PM +0200, Igor Mammedov wrote:
> > > On Mon, 4 Jun 2018 13:03:44 +0100
> > > Daniel P. Berrangé wrote:
> > >
> > > > The RUN_STATE_PRECONFIG state is not supposed to be reachable unless the
> > > > --preconfig argument is given to QEMU, but when it was introduced in:
> > > >
> > > > commit 047f7038f586d2150f16c6d9ba9cfd0479f0f6ac
> > > > Author: Igor Mammedov
> > > > Date: Fri May 11 19:24:43 2018 +0200
> > > >
> > > > cli: add --preconfig option
> > > >
> > > > ...the global 'current_run_state' variable was changed to have an
> > > > initial
> > > > value of RUN_STATE_PRECONFIG regardless of whether --preconfig is given.
> > > >
> > > > A second invokation of main_loop() was added which then toggles it back
> > > > to RUN_STATE_PRELAUNCH when --preconfig is not given. This is racy
> > > > because it leaves a window where QEMU is in RUN_STATE_PRECONFIG despite
> > > > --preconfig not being given.
> > > Above statements isn't exactly correct, PRECONFIG were supposed to be
> > > the new state of QEMU from start up till machine_run_board_init(),
> > > that would separate stage of initial configuration out from all
> > > encompassing PRELAUNCH state. So I'd scratch out above part.
> >
> > That doesn't really make sense, given that --preconfig is *not* the
> > default and thus not supposed to be an active state unless the app
> > has explicitly opted in.
> >
> > IMHO running PRECONFIG state for any period of time when the app
> > has not requested --preconfig is simply broken, and a recipe for
> > obscure bugs like the ones we've seen right now.
> mgmt hasn't opted in for default PRELAUNCH either, for it default
> PRECONFIG runstate is not visible unless it opts in so nothing
> is broken in regards to this.
> Default runstate selection is QEMU's internal impl. detail.
Daniel's description is how he expects it to work, but your
description reflects the way the state machine actually works
today (and how it worked befor the --preconfig series).
However, I agree with Daniel that moving to PRECONFIG or
PRELAUNCH if neither --preconfig or -S were specified is
confusing, and I would prefer to change it the way he suggests.
But:
[...]
> >$START -> PRELAUNCH {-> INMIGRATE]
> > | ^
> > | |
> > +-- PRECONFIG -+
> >
> > By marking the current state as PRECONFIG by default, we've essentially
> > given 2 meanings to PRECONFIG - sometimes it means stuff that can be
> > unconditionally run in early startup, and sometimes it means stuff that
> > can only be run if --preconfig is given. Introducing the separate NONE
> > state clarifies that inherant contradiction in startup phases.
> Yep, one can say it this way (as merged PRECONFIG was early
> configuration stage regardless of if it's unconditional early
> initialization or early CLI parsing/QMP configuration).
>
> Maybe adding NONE state makes sense but I'm not quite sure,
> that's why I'd not rush it in and discuss if we really need
> fragment early configuration into more stages.
> (we can do it later as both stages aren't visible to user by default).
Is it possible to fix the bugs first, and discuss how to refactor
the state machine later?
In the meantime, we could even document preconfig more accurately
to avoid additional confusion:
# @preconfig: Board initialization was not run yet. The state is
# visible to the outside only if the --preconfig CLI
# option is used. (Since 3.0)
--
Eduardo
Re: [Qemu-devel] [PATCH] spapr: don't call KVM_PPC_CONFIGURE_V3_MMU if HPT is in userspace
On Fri, May 25, 2018 at 02:54:12PM +0200, Greg Kurz wrote:
> Since the kernel commit "dbfcf3cb9c68 powerpc/64: Call H_REGISTER_PROC_TBL
> when running as a HPT guest on POWER9", a nested guest running with PR KVM
> hangs at boot:
>
> Preparing to boot Linux version 4.16.0-kvm-pr-hang-gku+
> (greg@qemu.boston16) (gcc version 8.1.1 20180502 (Red Hat 8.1.1-1) (GCC))
> #19 SMP Fri May 25 08:41:55 CEST 2018
> Detected machine type: 0101
> command line: root=UUID=22128c5c-30b1-4e0a-ac16-95853df31131 ro rhgb
> console=hvc0 early_printk disable-radix=on
> Max number of cores passed to firmware: 1024 (NR_CPUS = 1024)
> Calling ibm,client-architecture-support... done
> memory layout at init:
> memory_limit : (16 MB aligned)
> alloc_bottom : 01b8
> alloc_top: 3000
> alloc_top_hi : 0001
> rmo_top : 3000
> ram_top : 0001
> instantiating rtas at 0x2fff... done
> prom_hold_cpus: skipped
> copying OF device tree...
> Building dt strings...
> Building dt structure...
> Device tree strings 0x03d9 -> 0x03d90abb
> Device tree struct 0x03da -> 0x03db
> Quiescing Open Firmware ...
> Booting Linux via __start() @ 0x0040 ...
>
> This happens because the H_REGISTER_PROC_TBL implementation in QEMU
> always call KVM_PPC_CONFIGURE_V3_MMU when KVM is present. This fails
> in the case of PR KVM, which doesn't implement it, and QEMU returns
> H_PARAMETER to the guest, which is a BUG() condition in linux.
>
> In the case of PR, the HPT is allocated in userspace by QEMU, so it
> doesn't make sense to call KVM_PPC_CONFIGURE_V3_MMU in the first
> place. So, skip it in this case and let the guest boot.
>
> Signed-off-by: Greg Kurz
> ---
>
> Note that PR KVM requires this patch from Paul to work on POWER9:
>
> https://patchwork.ozlabs.org/patch/916766/
>
> The original request was coming from people who want to run openQA in
> fedora28 under PowerVM on a POWER9 system. This requires PR KVM, which
> will be running in HPT-mode since pHyp doesn't do radix.
>
> Cc'ing stable because fedora28 ships QEMU 2.11.x.
> ---
> hw/ppc/spapr_hcall.c |2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/hw/ppc/spapr_hcall.c b/hw/ppc/spapr_hcall.c
> index 022f6d810182..12cbb317e5e8 100644
> --- a/hw/ppc/spapr_hcall.c
> +++ b/hw/ppc/spapr_hcall.c
> @@ -1420,7 +1420,7 @@ static target_ulong h_register_process_table(PowerPCCPU
> *cpu,
>((flags & FLAG_GTSE) ? LPCR_GTSE : 0),
>LPCR_UPRT | LPCR_GTSE);
>
> -if (kvm_enabled()) {
> +if (kvm_enabled() && !spapr->htab) {
> return kvmppc_configure_v3_mmu(cpu, flags & FLAG_RADIX,
> flags & FLAG_GTSE, cproc);
Won't this also omit the configure MMU call if the guest is in radix
mode? We don't want that.
> }
>
--
David Gibson| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson
signature.asc
Description: PGP signature
Re: [Qemu-devel] [RFC PATCH] target/ppc: extend eieio for POWER9
On Mon, Jun 04, 2018 at 07:20:39PM +0200, Cédric Le Goater wrote: > POWER9 introduced a new variant of the eieio instruction using bit 6 > as a hint to tell the CPU it is a store-forwarding barrier. > > The usage of this eieio extension was recently added in Linux 4.17 > which activated the "support for a store forwarding barrier at kernel > entry/exit". > > This loosen the QEMU eieio instruction mask to boot newer kernel but I > think we should be adding a new *eieio* instruction specific to POWER9 > instead. I just don't know how to define an instruction variant with > the same op code for an ISA version. Any idea ? I think you're right that this should be done slightly differently. I think you can do that by adding a new instruction mask bit; say PPC2_MEM_EIEIO2 or whatever. You leave the existing GEN_HANDLER as is, add another GEN_HANDLER_E with the new mask dependent on the new bit, then make sure POWER9 has the new bit set, but not the old one. > > Signed-off-by: Cédric Le Goater > --- > > target/ppc/translate.c |2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > Index: qemu-powernv-2.13.git/target/ppc/translate.c > === > --- qemu-powernv-2.13.git.orig/target/ppc/translate.c > +++ qemu-powernv-2.13.git/target/ppc/translate.c > @@ -6496,7 +6496,7 @@ GEN_HANDLER(lswi, 0x1F, 0x15, 0x12, 0x00 > GEN_HANDLER(lswx, 0x1F, 0x15, 0x10, 0x0001, PPC_STRING), > GEN_HANDLER(stswi, 0x1F, 0x15, 0x16, 0x0001, PPC_STRING), > GEN_HANDLER(stswx, 0x1F, 0x15, 0x14, 0x0001, PPC_STRING), > -GEN_HANDLER(eieio, 0x1F, 0x16, 0x1A, 0x03FFF801, PPC_MEM_EIEIO), > +GEN_HANDLER(eieio, 0x1F, 0x16, 0x1A, 0x01FFF801, PPC_MEM_EIEIO), > GEN_HANDLER(isync, 0x13, 0x16, 0x04, 0x03FFF801, PPC_MEM), > GEN_HANDLER_E(lbarx, 0x1F, 0x14, 0x01, 0, PPC_NONE, PPC2_ATOMIC_ISA206), > GEN_HANDLER_E(lharx, 0x1F, 0x14, 0x03, 0, PPC_NONE, PPC2_ATOMIC_ISA206), > -- David Gibson| I'll have my music baroque, and my code david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_ | _way_ _around_! http://www.ozlabs.org/~dgibson signature.asc Description: PGP signature
Re: [Qemu-devel] [PATCH 1/6] ahci: move PIO Setup FIS before transfer, fix it for ATAPI commands
On 06/04/2018 11:50 AM, Paolo Bonzini wrote: > On 02/06/2018 03:22, John Snow wrote: >> - Status: Should be the status register after receiving the H2D Register >> update FIS, but prior to the data transfer, I think. "New value of the >> Status register of the Command Block for initiation of host data >> transfer." >> I think this is being set correctly after this patch. >> >> - Error: "Contains the new value of the Error register of the Command >> Block at the conclusion of all subsequent Data to Device frames." >> >> This is why we were sending out post-hoc PIO Setup FIS frames before, >> how would I know what the error register *will* be...? What? > > You don't, I guess. Zero? > Yeah, I don't mean to hold it up based on other arcane stuff, I was just briefly hoping that maybe you actually understood it so I could fix it once and for all... >> - LBA: Presumably unimportant for the purposes of receiving a command >> PACKET, as we won't be writing it to disk, but a buffer. The values >> can be reported dutifully. >> >> - Device: Just report the register value dutifully. >> >> - Count: Likely just relays 0, as the H2D REG FIS should have updated >> that to zero as part of the PACKET command, as per ATA8 ACS3 7.21.3. >> In any case, just report the register value dutifully. >> >> - E_Status: "Contains the new value of the Status register of the >> Command Block at the conclusion of the subsequent Data FIS." >> >> Again, how would I know...? >> >> - Transfer Count: Should be 12, as per what we specified in 0xA1 >> IDENTIFY PACKET DEVICE, see core.c line 234. Your patch gets this >> correct, as we'll actually report the PIO FIS for the packet itself. >> >> >> What this patch does do, though, is change the context of the Status, >> Error and E_Status registers to something different. Everything else >> should be the same, but I'd feel better about taking this patch if I >> understood what exactly this FIS packet was supposed to convey, but I don't. > > At least Status and Transfer Count are correct after this patch. :/ > > Paolo > How about ... https://github.com/jnsnow/qemu/commit/0657390a2639b7cb533ca8b514c49c0edd3f4483 This sends out a PIO Setup FIS for every PIO transfer and adjusts the tests accordingly. Presumably any sane driver gets end of transfer status from the D2H Register Update FIS and ... probably ignores the PIO Setup FIS entirely. I *hope*. (Certainly with how wrong we've gotten it, it seems very likely that nobody uses this.) It's probably still wrong for the reasons I've outlined in my initial reply here, but it's probably less wrong. I can't think of a reason you'd want an AHCI device to interrupt so much, but it's my sincere hope that (1) No sane AHCI driver uses PIO, and (2) If it does, it does not do so with the PSFIS interrupt on ... *shrug* --js
Re: [Qemu-devel] [PATCH v3 2/3] WHPX: dynamically load WHP libraries
Paolo - I saw that this merged with commit:
327fccb288976f95808efa968082fc9d4a9ced84 but it seems to be missing
whp-dispatch.h so now the build is failing. Any idea how this file failed to
get included?
Thanks,
Justin
> -Original Message-
> From: petrutlucia...@gmail.com
> Sent: Friday, May 25, 2018 7:02 AM
> To: qemu-devel@nongnu.org
> Cc: Lucian Petrut ; apilotti
> ; Justin Terry (VM)
>
> Subject: [PATCH v3 2/3] WHPX: dynamically load WHP libraries
>
> From: Lucian Petrut
>
> We're currently linking against import libraries of the WHP DLLs.
>
> By dynamically loading the libraries, we ensure that QEMU will work on
> previous Windows versions, where the WHP DLLs will be missing (assuming
> that WHP is not requested).
>
> Also, we're simplifying the build process, as we no longer require the import
> libraries.
>
> Signed-off-by: Alessandro Pilotti
> Signed-off-by: Justin Terry (VM)
> Signed-off-by: Lucian Petrut
> ---
> configure | 15 +--
> target/i386/whp-dispatch.h | 56
> target/i386/whpx-all.c | 222 ++
> ---
> 3 files changed, 206 insertions(+), 87 deletions(-) create mode 100644
> target/i386/whp-dispatch.h
>
> diff --git a/configure b/configure
> index a8498ab..99b4a28 100755
> --- a/configure
> +++ b/configure
> @@ -2524,20 +2524,7 @@ fi
> ##
> # Windows Hypervisor Platform accelerator (WHPX) check if test "$whpx" !=
> "no" ; then
> -cat > $TMPC << EOF
> -#include
> -#include
> -#include
> -int main(void) {
> -WHV_CAPABILITY whpx_cap;
> -UINT32 writtenSize;
> -WHvGetCapability(WHvCapabilityCodeFeatures, _cap,
> sizeof(whpx_cap),
> - );
> -return 0;
> -}
> -EOF
> -if compile_prog "" "-lWinHvPlatform -lWinHvEmulation" ; then
> -libs_softmmu="$libs_softmmu -lWinHvPlatform -lWinHvEmulation"
> +if check_include "WinHvPlatform.h" && check_include
> + "WinHvEmulation.h"; then
> whpx="yes"
> else
> if test "$whpx" = "yes"; then
> diff --git a/target/i386/whp-dispatch.h b/target/i386/whp-dispatch.h new file
> mode 100644 index 000..d8d3485
> --- /dev/null
> +++ b/target/i386/whp-dispatch.h
> @@ -0,0 +1,56 @@
> +#include "windows.h"
> +#include
> +
> +#include
> +#include
> +
> +#ifndef WHP_DISPATCH_H
> +#define WHP_DISPATCH_H
> +
> +
> +#define LIST_WINHVPLATFORM_FUNCTIONS(X) \
> + X(HRESULT, WHvGetCapability, (WHV_CAPABILITY_CODE CapabilityCode,
> +VOID* CapabilityBuffer, UINT32 CapabilityBufferSizeInBytes, UINT32*
> +WrittenSizeInBytes)) \
> + X(HRESULT, WHvCreatePartition, (WHV_PARTITION_HANDLE* Partition)) \
> + X(HRESULT, WHvSetupPartition, (WHV_PARTITION_HANDLE Partition)) \
> + X(HRESULT, WHvDeletePartition, (WHV_PARTITION_HANDLE Partition)) \
> + X(HRESULT, WHvGetPartitionProperty, (WHV_PARTITION_HANDLE
> Partition,
> +WHV_PARTITION_PROPERTY_CODE PropertyCode, VOID* PropertyBuffer,
> UINT32
> +PropertyBufferSizeInBytes, UINT32* WrittenSizeInBytes)) \
> + X(HRESULT, WHvSetPartitionProperty, (WHV_PARTITION_HANDLE
> Partition,
> +WHV_PARTITION_PROPERTY_CODE PropertyCode, const VOID*
> PropertyBuffer,
> +UINT32 PropertyBufferSizeInBytes)) \
> + X(HRESULT, WHvMapGpaRange, (WHV_PARTITION_HANDLE Partition,
> VOID*
> +SourceAddress, WHV_GUEST_PHYSICAL_ADDRESS GuestAddress, UINT64
> +SizeInBytes, WHV_MAP_GPA_RANGE_FLAGS Flags)) \
> + X(HRESULT, WHvUnmapGpaRange, (WHV_PARTITION_HANDLE Partition,
> +WHV_GUEST_PHYSICAL_ADDRESS GuestAddress, UINT64 SizeInBytes)) \
> + X(HRESULT, WHvTranslateGva, (WHV_PARTITION_HANDLE Partition,
> UINT32
> +VpIndex, WHV_GUEST_VIRTUAL_ADDRESS Gva,
> WHV_TRANSLATE_GVA_FLAGS
> +TranslateFlags, WHV_TRANSLATE_GVA_RESULT* TranslationResult,
> +WHV_GUEST_PHYSICAL_ADDRESS* Gpa)) \
> + X(HRESULT, WHvCreateVirtualProcessor, (WHV_PARTITION_HANDLE
> +Partition, UINT32 VpIndex, UINT32 Flags)) \
> + X(HRESULT, WHvDeleteVirtualProcessor, (WHV_PARTITION_HANDLE
> +Partition, UINT32 VpIndex)) \
> + X(HRESULT, WHvRunVirtualProcessor, (WHV_PARTITION_HANDLE
> Partition,
> +UINT32 VpIndex, VOID* ExitContext, UINT32 ExitContextSizeInBytes)) \
> + X(HRESULT, WHvCancelRunVirtualProcessor, (WHV_PARTITION_HANDLE
> +Partition, UINT32 VpIndex, UINT32 Flags)) \
> + X(HRESULT, WHvGetVirtualProcessorRegisters,
> (WHV_PARTITION_HANDLE
> +Partition, UINT32 VpIndex, const WHV_REGISTER_NAME* RegisterNames,
> +UINT32 RegisterCount, WHV_REGISTER_VALUE* RegisterValues)) \
> + X(HRESULT, WHvSetVirtualProcessorRegisters, (WHV_PARTITION_HANDLE
> +Partition, UINT32 VpIndex, const WHV_REGISTER_NAME* RegisterNames,
> +UINT32 RegisterCount, const WHV_REGISTER_VALUE* RegisterValues)) \
> +
> +
> +#define LIST_WINHVEMULATION_FUNCTIONS(X) \
> + X(HRESULT, WHvEmulatorCreateEmulator, (const
> WHV_EMULATOR_CALLBACKS*
> +Callbacks, WHV_EMULATOR_HANDLE* Emulator)) \
> + X(HRESULT, WHvEmulatorDestroyEmulator, (WHV_EMULATOR_HANDLE
> +Emulator)) \
> + X(HRESULT,
Re: [Qemu-devel] [PATCH 7/8] sdcard: Reflect when the Spec v3 is supported in the Config Register (SCR)
On Sat, Jun 2, 2018 at 5:08 PM, Philippe Mathieu-Daudé wrote:
> Signed-off-by: Philippe Mathieu-Daudé
Reviewed-by: Alistair Francis
Alistair
> ---
> include/hw/sd/sd.h | 1 +
> hw/sd/sd.c | 7 +--
> 2 files changed, 6 insertions(+), 2 deletions(-)
>
> diff --git a/include/hw/sd/sd.h b/include/hw/sd/sd.h
> index 7c6ad3c8f1..b865aafc33 100644
> --- a/include/hw/sd/sd.h
> +++ b/include/hw/sd/sd.h
> @@ -57,6 +57,7 @@
> enum SDPhySpecificationVersion {
> SD_PHY_SPECv1_10_VERS = 1,
> SD_PHY_SPECv2_00_VERS = 2,
> +SD_PHY_SPECv3_01_VERS = 3,
> };
>
> typedef enum {
> diff --git a/hw/sd/sd.c b/hw/sd/sd.c
> index da65f2b178..83426da133 100644
> --- a/hw/sd/sd.c
> +++ b/hw/sd/sd.c
> @@ -315,11 +315,14 @@ static void sd_set_scr(SDState *sd)
> if (sd->spec_version == SD_PHY_SPECv1_10_VERS) {
> sd->scr[0] |= 1;/* Spec Version 1.10 */
> } else {
> -sd->scr[0] |= 2;/* Spec Version 2.00 */
> +sd->scr[0] |= 2;/* Spec Version 2.00 or Version 3.0X */
> }
> sd->scr[1] = (2 << 4) /* SDSC Card (Security Version 1.01) */
> | 0b0101; /* 1-bit or 4-bit width bus modes */
> sd->scr[2] = 0x00; /* Extended Security is not supported. */
> +if (sd->spec_version >= SD_PHY_SPECv3_01_VERS) {
> +sd->scr[2] |= 1 << 7; /* Spec Version 3.0X */
> +}
> sd->scr[3] = 0x00;
> /* reserved for manufacturer usage */
> sd->scr[4] = 0x00;
> @@ -2067,7 +2070,7 @@ static void sd_realize(DeviceState *dev, Error **errp)
>
> switch (sd->spec_version) {
> case SD_PHY_SPECv1_10_VERS
> - ... SD_PHY_SPECv2_00_VERS:
> + ... SD_PHY_SPECv3_01_VERS:
> break;
> default:
> error_setg(errp, "Invalid SD card Spec version: %u",
> sd->spec_version);
> --
> 2.17.1
>
>
Re: [Qemu-devel] [PATCH 03/17] iotests: ask qemu for supported formats
On 06/04/2018 05:34 AM, Thomas Huth wrote: On 04.06.2018 09:18, Markus Armbruster wrote: Roman Kagan writes: Add helper functions to query the block drivers actually supported by QEMU using "-drive format=?". This allows to skip certain tests that require drivers not built in or whitelisted in QEMU. +supported_formats=$($QEMU_PROG $QEMU_OPTIONS -drive format=\? 2>&1 | \ Use of '?' to get help is deprecated. Please use 'format=help', and update your commit message accordingly. Is it? Where did we document that? Hmm, we haven't documented it yet, but it's been that way since commit c8057f95, in Aug 2012, when we added 'help' as the preferred synonym, and have tried to avoid mention of '?' in new documentation (as it requires additional shell quoting). I'm guessing we'll probably see a patch from you to start an official deprecation window? -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org
Re: [Qemu-devel] [PATCH 05/12] migration: show the statistics of compression
On 06/04/2018 04:55 AM, guangrong.x...@gmail.com wrote:
From: Xiao Guangrong
Then the uses can adjust the parameters based on this info
Currently, it includes:
pages: amount of pages compressed and transferred to the target VM
busy: amount of count that no free thread to compress data
busy-rate: rate of thread busy
reduced-size: amount of bytes reduced by compression
compression-rate: rate of compressed size
Signed-off-by: Xiao Guangrong
---
+++ b/qapi/migration.json
@@ -72,6 +72,26 @@
'cache-miss': 'int', 'cache-miss-rate': 'number',
'overflow': 'int' } }
+##
+# @CompressionStats:
+#
+# Detailed compression migration statistics
Sounds better as s/compression migration/migration compression/
+#
+# @pages: amount of pages compressed and transferred to the target VM
+#
+# @busy: amount of count that no free thread to compress data
Not sure what was meant, maybe:
@busy: count of times that no free thread was available to compress data
+#
+# @busy-rate: rate of thread busy
In what unit? pages per second?
+#
+# @reduced-size: amount of bytes reduced by compression
+#
+# @compression-rate: rate of compressed size
In what unit?
+#
+##
Missing a 'Since: 3.0' tag
+{ 'struct': 'CompressionStats',
+ 'data': {'pages': 'int', 'busy': 'int', 'busy-rate': 'number',
+ 'reduced-size': 'int', 'compression-rate': 'number' } }
+
##
# @MigrationStatus:
#
@@ -169,6 +189,8 @@
# only present when the postcopy-blocktime migration capability
# is enabled. (Since 2.13)
Pre-existing - we need to fix this 2.13 to be 3.0 (if it isn't already
fixed)
#
+# @compression: compression migration statistics, only returned if compression
+# feature is on and status is 'active' or 'completed' (Since 2.14)
There will not be a 2.14 (for that matter, not even a 2.13). The next
release is 3.0.
#
# Since: 0.14.0
##
@@ -183,7 +205,8 @@
'*cpu-throttle-percentage': 'int',
'*error-desc': 'str',
'*postcopy-blocktime' : 'uint32',
- '*postcopy-vcpu-blocktime': ['uint32']} }
+ '*postcopy-vcpu-blocktime': ['uint32'],
+ '*compression': 'CompressionStats'} }
##
# @query-migrate:
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3266
Virtualization: qemu.org | libvirt.org
Re: [Qemu-devel] [PATCH V8 11/17] qapi: Add new command to query colo status
On 06/03/2018 12:05 AM, Zhang Chen wrote:
Libvirt or other high level software can use this command query colo status.
You can test this command like that:
{'execute':'query-colo-status'}
Signed-off-by: Zhang Chen
---
+++ b/qapi/migration.json
@@ -1231,6 +1231,40 @@
##
{ 'command': 'xen-colo-do-checkpoint' }
+##
+# @COLOStatus:
+#
+# The result format for 'query-colo-status'.
+#
+# @mode: COLO running mode. If COLO is running, this field will return
+#'primary' or 'secodary'.
s/secodary/secondary/
+#
+# @colo-running: true if COLO is running.
+#
+# @reason: describes the reason for the COLO exit.
+#
+# Since: 2.13
3.0
+##
+{ 'struct': 'COLOStatus',
+ 'data': { 'mode': 'COLOMode', 'colo-running': 'bool', 'reason':
'COLOExitReason' } }
+
+##
+# @query-colo-status:
+#
+# Query COLO status while the vm is running.
+#
+# Returns: A @COLOStatus object showing the status.
+#
+# Example:
+#
+# -> { "execute": "query-colo-status" }
+# <- { "return": { "mode": "primary", "colo-running": true, "reason":
"request" } }
+#
+# Since: 2.13
3.0
+##
+{ 'command': 'query-colo-status',
+ 'returns': 'COLOStatus' }
+
##
# @migrate-recover:
#
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3266
Virtualization: qemu.org | libvirt.org
Re: [Qemu-devel] [PATCH V8 10/17] qmp event: Add COLO_EXIT event to notify users while exited COLO
On 06/03/2018 12:05 AM, Zhang Chen wrote:
From: zhanghailiang
If some errors happen during VM's COLO FT stage, it's important to
notify the users of this event. Together with 'x-colo-lost-heartbeat',
Users can intervene in COLO's failover work immediately.
If users don't want to get involved in COLO's failover verdict,
it is still necessary to notify users that we exited COLO mode.
Signed-off-by: zhanghailiang
Signed-off-by: Li Zhijian
Signed-off-by: Zhang Chen
Reviewed-by: Eric Blake
---
migration/colo.c| 31 +++
qapi/migration.json | 38 ++
2 files changed, 69 insertions(+)
diff --git a/migration/colo.c b/migration/colo.c
index c083d3696f..bedb677788 100644
--- a/migration/colo.c
+/*
+ * There are only two reasons we can go here, some error happened.
+ * Or the user triggered failover.
s/go here/get here/
s/happened. Or/happened, or/
+++ b/qapi/migration.json
@@ -880,6 +880,44 @@
{ 'enum': 'FailoverStatus',
'data': [ 'none', 'require', 'active', 'completed', 'relaunch' ] }
+##
+# @COLO_EXIT:
+#
+# Emitted when VM finishes COLO mode due to some errors happening or
+# at the request of users.
+#
+# @mode: report COLO mode when COLO exited.
+#
+# @reason: describes the reason for the COLO exit.
+#
+# Since: 2.13
s/2.13/3.0/
+#
+# Example:
+#
+# <- { "timestamp": {"seconds": 2032141960, "microseconds": 417172},
+# "event": "COLO_EXIT", "data": {"mode": "primary", "reason": "request" }
}
+#
+##
+{ 'event': 'COLO_EXIT',
+ 'data': {'mode': 'COLOMode', 'reason': 'COLOExitReason' } }
+
+##
+# @COLOExitReason:
+#
+# The reason for a COLO exit
+#
+# @none: no failover has ever happened, This can't occur in the COLO_EXIT
event,
s/happened,/happened./
+# only in the result of query-colo-status.
+#
+# @request: COLO exit is due to an external request
+#
+# @error: COLO exit is due to an internal error
+#
+# Since: 2.13
s/2.13/3.0/
+##
+{ 'enum': 'COLOExitReason',
+ 'data': [ 'none', 'request', 'error' ] }
+
##
# @x-colo-lost-heartbeat:
#
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3266
Virtualization: qemu.org | libvirt.org
Re: [Qemu-devel] [RFC] qapi: command category to stimulate high-level machine devices
On 06/01/2018 10:16 PM, Steffen Görtz wrote:
From: Steffen Goertz
Add a new category "stimulate" to host commands that
act upon high-level devices associated with machines/boards.
This is patch is part of an effort to add support for BBC:microbit
educational computer to QEMU.
The microbit board, as many other microcontroller boards,
contains typical trivial (digital) input and output options such as buttons and
LEDs,
but also more sophiscated possibilities such as analog inputs and
s/sophiscated/sophisticated/
complex dedicated sensor ICs that are connected to the microbit machine
by means of inter-ic bus (i2c).
These devices interact with peripheral devices of the microcontroller
in use, for example with the GPIO peripheral of the used NRF51.
One aim of our efforts is to create a user interface that models the
look and feel of the physical microbit board and lets the user interact
with its inputs and provide feedback on its outsputs.
s/outsputs/outputs/
For this it is necessary to transmit user inputs such as the pressing of a
button to the machine.
In return, when the state of an output is changed on the machine, this
change needs to be reflected in the user interface.
At the moment, there are only a few QMP commands that provide user input to the
machine, eg. send-keys, input-button. These commands require very
high-level concepts, which are not really suitable for applications that
microcontrollers are typically used in in.
This RFC is meant to start a discussion on how to provide stimuli to
low-complex inputs and outputs typically found in embedded
microcontroller applications. To the best of my knowledge, there are
currently no examples of how to provide such stimuli to either SoC
device peripheral or machine level concepts like pushbuttons and LEDs.
To my understanding, the following concepts/apis are needed:
- QMP commands to send stimuli to machine level concepts like
pushbuttons
- Machine level devices that receive these stimuli and act as an adapter
to manipulate the associated SoC peripheral devices
- For outputs, machine level devices are needed that observe changes in
SoC peripherals and emit QMP events that clients can receive
This patch adds a new qmp command "buttons-set-state" to update the
push-down state of arbitrarily identified buttons (string identifier).
An arbitrary string has the most flexibility, but also the least
discoverability. Is there any way to enumerate which strings are valid,
and/or make the button names an enum instead of an open-coded string?
The handler of this command is mocked to set the machines SoC gpio/irq
lines associated with these buttons on the machine by means of object
property aliases. This is just a mock until a proper concept/api is
agreed upon.
As i recently joined the QEMU community as part of GSoC, i am still a
greenhorn to the codebase and i am really excited to discuss potential
concepts and APIs to realize the described features.
This last paragraph...
Based-on: <20180503090532.3113-1-j...@jms.id.au>
Signed-off-by: Steffen Goertz
---
...would fit better here, after the --- separator. It is useful to
reviewers now, but a year from now when reading the git history (and
hopefully after you are a regular contributor), it won't be as relevant
to understanding this particular patch or your contributions in general.
At this point, I haven't yet decided if this interface addition is the
best way to approach things, but if we take it without a redesign, it
needs at least the following tweaks:
+++ b/qapi/stimulate.json
@@ -0,0 +1,24 @@
+# -*- Mode: Python -*-
+
+##
+# @ButtonPress:
+#
+# @identifier: Name of the button.
+# @pushed_down: State of the button.
In QMP, we prefer naming things with dash, as in 'pushed-down'. Or, if
it is easier, you could create an enum with values 'down' and 'up' and
use that, instead of 'bool', as the type, at which point this variable
might be better named as 'position' instead of 'pushed-down'.
+#
+##
Missing a tag of:
Since: 3.0
+{ 'struct': 'ButtonPress',
+ 'data': {
+'identifier': 'str',
+'pushed_down': 'bool'
+ } }
+
+##
+# @buttons-set-state:
+#
+# @buttons: List of updated button states.
+#
+# Returns: nothing in case of success
+##
Missing a Since: tag.
+{ 'command': 'buttons-set-state',
+ 'data': { 'buttons': ['ButtonPress'] } }
Doesn't let a user set button state (presumably that's for a later
patch), but at least answers the question on introspecting the names of
which buttons have state.
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3266
Virtualization: qemu.org | libvirt.org
Re: [Qemu-devel] [PATCH RESEND v1 0/3] add support for VCPU event states
> On 1 June 2018 at 15:16, gengdongjiu wrote: > > Hi Peter, > > >> You should wait for that to get into master (it is in Paolo's most > >> recent patchset but that has an issue with a different patch, so is > >> waiting on Paolo to do a respin). Then you can base on that. At that > >> point a simple script update should work fine (if you need anything > >> in rc7 that wasn't in rc6). > > > > Ok, got it. Thanks a lot for the explanation, I will wait for your > > patch[1] merge into master and based on it. > > The rc6 header update is now in QEMU git master. Ok, thanks for the reminder. I will rebased it. > > thanks > -- PMM
Re: [Qemu-devel] [PATCH v2 2/2] vl: fix use of --daemonize with --preconfig
On Mon, 4 Jun 2018 13:03:45 +0100
Daniel P. Berrangé wrote:
> When using --daemonize, the initial lead process will fork a child and
> then wait to be notified that setup is complete via a pipe, before it
> exits. When using --preconfig there is an extra call to main_loop()
> before the notification is done from os_setup_post(). Thus the parent
> process won't exit until the mgmt application connects to the monitor
> and tells QEMU to leave the RUN_STATE_PRECONFIG. The mgmt application
> won't connect to the monitor until daemonizing has completed though.
>
> This is a chicken and egg problem, leading to deadlock at startup.
>
> The only viable way to fix this is to call os_setup_post() before
> the early main_loop() call when in RUN_STATE_PRECONFIG. This has the
> downside that any errors from this point onwards won't be handled
> well by the mgmt application, because it will think QEMU has started
> successfully, so not be expecting an abrupt exit. The only way to
> deal with that is to move as much user input validation as possible
> to before the main_loop() call. This is left as an exercise for
> future interested developers.
>
> Signed-off-by: Daniel P. Berrangé
[...]
How about combining ideas from yours and Michal's patches?
It should fix broken iotests/libvirt sync point and
we can think about NONE runstate idea at without rushing it.
If it looks acceptable, I'll post proper patches + test case for it
after some testing (to ensure that iotests Max pointed out are working
as expected)
diff --git a/vl.c b/vl.c
index c4fe255..a2062d6 100644
--- a/vl.c
+++ b/vl.c
@@ -1953,10 +1953,15 @@ static bool main_loop_should_exit(void)
static void main_loop(void)
{
+static bool os_setup_post_done = false;
#ifdef CONFIG_PROFILER
int64_t ti;
#endif
-do {
+if (!os_setup_post_done) {
+os_setup_post();
+os_setup_post_done = true;
+}
+while (!main_loop_should_exit()) {
#ifdef CONFIG_PROFILER
ti = profile_getclock();
#endif
@@ -1964,7 +1969,7 @@ static void main_loop(void)
#ifdef CONFIG_PROFILER
dev_time += profile_getclock() - ti;
#endif
-} while (!main_loop_should_exit());
+}
}
static void version(void)
[Qemu-devel] [PATCH v2] tcg-target.inc.c: Use byte form of xgetbv instruction
Signed-off-by: John Arbuckle
---
v2 changes:
- Fixed a spacing issue in the asm() function.
tcg/i386/tcg-target.inc.c | 6 +-
1 file changed, 5 insertions(+), 1 deletion(-)
diff --git a/tcg/i386/tcg-target.inc.c b/tcg/i386/tcg-target.inc.c
index 5357909fff..09141fa8e0 100644
--- a/tcg/i386/tcg-target.inc.c
+++ b/tcg/i386/tcg-target.inc.c
@@ -3501,7 +3501,11 @@ static void tcg_target_init(TCGContext *s)
sure of not hitting invalid opcode. */
if (c & bit_OSXSAVE) {
unsigned xcrl, xcrh;
-asm ("xgetbv" : "=a" (xcrl), "=d" (xcrh) : "c" (0));
+/*
+ * The xgetbv instruction is not available to older versions of the
+ * assembler, so we encode the instruction manually.
+ */
+asm(".byte 0x0f, 0x01, 0xd0" : "=a" (xcrl), "=d" (xcrh) : "c" (0));
if ((xcrl & 6) == 6) {
have_avx1 = (c & bit_AVX) != 0;
have_avx2 = (b7 & bit_AVX2) != 0;
--
2.14.3 (Apple Git-98)
Re: [Qemu-devel] [Qemu-block] [PATCH v2 5/8] qcow: Switch to a byte-based driver
On Thu, May 31, 2018 at 03:50:43PM -0500, Eric Blake wrote:
> We are gradually moving away from sector-based interfaces, towards
> byte-based. The qcow driver is now ready to fully utilize the
> byte-based callback interface, as long as we override the default
> alignment to still be 512 (needed at least for asserts present
> because of encryption, but easier to do everywhere than to audit
> which sub-sector requests are handled correctly, especially since
> we no longer recommend qcow for new disk images).
>
> Signed-off-by: Eric Blake
>
> ---
> v2: minor rebase to changes earlier in series
> ---
> block/qcow.c | 35 ---
> 1 file changed, 20 insertions(+), 15 deletions(-)
>
> diff --git a/block/qcow.c b/block/qcow.c
> index 44adeba276c..55720b006ef 100644
> --- a/block/qcow.c
> +++ b/block/qcow.c
> @@ -69,7 +69,6 @@ typedef struct QCowHeader {
> typedef struct BDRVQcowState {
> int cluster_bits;
> int cluster_size;
> -int cluster_sectors;
> int l2_bits;
> int l2_size;
> unsigned int l1_size;
> @@ -235,7 +234,6 @@ static int qcow_open(BlockDriverState *bs, QDict
> *options, int flags,
> }
> s->cluster_bits = header.cluster_bits;
> s->cluster_size = 1 << s->cluster_bits;
> -s->cluster_sectors = 1 << (s->cluster_bits - 9);
> s->l2_bits = header.l2_bits;
> s->l2_size = 1 << s->l2_bits;
> bs->total_sectors = header.size / 512;
> @@ -613,8 +611,18 @@ static int decompress_cluster(BlockDriverState *bs,
> uint64_t cluster_offset)
> return 0;
> }
>
> -static coroutine_fn int qcow_co_readv(BlockDriverState *bs, int64_t
> sector_num,
> - int nb_sectors, QEMUIOVector *qiov)
> +static void qcow_refresh_limits(BlockDriverState *bs, Error **errp)
> +{
> +/* At least encrypted images require 512-byte alignment. Apply the
> + * limit universally, rather than just on encrypted images, as
> + * it's easier to let the block layer handle rounding than to
> + * audit this code further. */
> +bs->bl.request_alignment = BDRV_SECTOR_SIZE;
> +}
> +
> +static coroutine_fn int qcow_co_preadv(BlockDriverState *bs, uint64_t offset,
> + uint64_t bytes, QEMUIOVector *qiov,
> + int flags)
> {
> BDRVQcowState *s = bs->opaque;
> int offset_in_cluster;
> @@ -624,9 +632,8 @@ static coroutine_fn int qcow_co_readv(BlockDriverState
> *bs, int64_t sector_num,
> QEMUIOVector hd_qiov;
> uint8_t *buf;
> void *orig_buf;
> -int64_t offset = sector_num * BDRV_SECTOR_SIZE;
> -int64_t bytes = nb_sectors * BDRV_SECTOR_SIZE;
>
> +assert(!flags);
Why this assert here and in the _pwritev()?
> if (qiov->niov > 1) {
> buf = orig_buf = qemu_try_blockalign(bs, qiov->size);
> if (buf == NULL) {
> @@ -718,9 +725,9 @@ static coroutine_fn int qcow_co_readv(BlockDriverState
> *bs, int64_t sector_num,
> return ret;
> }
>
> -static coroutine_fn int qcow_co_writev(BlockDriverState *bs, int64_t
> sector_num,
> - int nb_sectors, QEMUIOVector *qiov,
> - int flags)
> +static coroutine_fn int qcow_co_pwritev(BlockDriverState *bs, uint64_t
> offset,
> +uint64_t bytes, QEMUIOVector *qiov,
> +int flags)
> {
> BDRVQcowState *s = bs->opaque;
> int offset_in_cluster;
> @@ -730,8 +737,6 @@ static coroutine_fn int qcow_co_writev(BlockDriverState
> *bs, int64_t sector_num,
> QEMUIOVector hd_qiov;
> uint8_t *buf;
> void *orig_buf;
> -int64_t offset = sector_num * BDRV_SECTOR_SIZE;
> -int64_t bytes = nb_sectors * BDRV_SECTOR_SIZE;
>
> assert(!flags);
> s->cluster_cache_offset = -1; /* disable compressed cache */
> @@ -1108,8 +1113,7 @@ qcow_co_pwritev_compressed(BlockDriverState *bs,
> uint64_t offset,
>
> if (ret != Z_STREAM_END || out_len >= s->cluster_size) {
> /* could not compress: write normal cluster */
> -ret = qcow_co_writev(bs, offset >> BDRV_SECTOR_BITS,
> - bytes >> BDRV_SECTOR_BITS, qiov, 0);
> +ret = qcow_co_pwritev(bs, offset, bytes, qiov, 0);
> if (ret < 0) {
> goto fail;
> }
> @@ -1194,9 +1198,10 @@ static BlockDriver bdrv_qcow = {
> .bdrv_co_create_opts= qcow_co_create_opts,
> .bdrv_has_zero_init = bdrv_has_zero_init_1,
> .supports_backing = true,
> +.bdrv_refresh_limits= qcow_refresh_limits,
>
> -.bdrv_co_readv = qcow_co_readv,
> -.bdrv_co_writev = qcow_co_writev,
> +.bdrv_co_preadv = qcow_co_preadv,
> +.bdrv_co_pwritev= qcow_co_pwritev,
> .bdrv_co_block_status = qcow_co_block_status,
>
> .bdrv_make_empty= qcow_make_empty,
> --
> 2.14.3
>
>
Re: [Qemu-devel] [Qemu-block] [PATCH v2 4/8] qcow: Switch qcow_co_writev to byte-based calls
On Thu, May 31, 2018 at 03:50:42PM -0500, Eric Blake wrote:
> We are gradually moving away from sector-based interfaces, towards
> byte-based. Make the change for the internals of the qcow
> driver write function, by iterating over offset/bytes instead of
> sector_num/nb_sectors, and with a rename of index_in_cluster and
> repurposing of n to track bytes instead of sectors.
>
> A later patch will then switch the qcow driver as a whole over
> to byte-based operation.
>
> Signed-off-by: Eric Blake
>
> ---
> v2: prefer 64-bit * over 23-bit <<, rename variable for legibility [Kevin]
> ---
> block/qcow.c | 36 +---
> 1 file changed, 17 insertions(+), 19 deletions(-)
>
> diff --git a/block/qcow.c b/block/qcow.c
> index b90915218ff..44adeba276c 100644
> --- a/block/qcow.c
> +++ b/block/qcow.c
> @@ -723,13 +723,15 @@ static coroutine_fn int qcow_co_writev(BlockDriverState
> *bs, int64_t sector_num,
> int flags)
> {
> BDRVQcowState *s = bs->opaque;
> -int index_in_cluster;
> +int offset_in_cluster;
> uint64_t cluster_offset;
> int ret = 0, n;
> struct iovec hd_iov;
> QEMUIOVector hd_qiov;
> uint8_t *buf;
> void *orig_buf;
> +int64_t offset = sector_num * BDRV_SECTOR_SIZE;
> +int64_t bytes = nb_sectors * BDRV_SECTOR_SIZE;
>
> assert(!flags);
> s->cluster_cache_offset = -1; /* disable compressed cache */
> @@ -749,16 +751,14 @@ static coroutine_fn int qcow_co_writev(BlockDriverState
> *bs, int64_t sector_num,
>
> qemu_co_mutex_lock(>lock);
>
> -while (nb_sectors != 0) {
> -
> -index_in_cluster = sector_num & (s->cluster_sectors - 1);
> -n = s->cluster_sectors - index_in_cluster;
> -if (n > nb_sectors) {
> -n = nb_sectors;
> +while (bytes != 0) {
> +offset_in_cluster = offset & (s->cluster_size - 1);
> +n = s->cluster_size - offset_in_cluster;
> +if (n > bytes) {
> +n = bytes;
> }
> -ret = get_cluster_offset(bs, sector_num << 9, 1, 0,
> - index_in_cluster << 9,
> - (index_in_cluster + n) << 9,
> _offset);
> +ret = get_cluster_offset(bs, offset, 1, 0, offset_in_cluster,
> + offset_in_cluster + n, _offset);
> if (ret < 0) {
> break;
> }
> @@ -768,30 +768,28 @@ static coroutine_fn int qcow_co_writev(BlockDriverState
> *bs, int64_t sector_num,
> }
> if (bs->encrypted) {
> assert(s->crypto);
> -if (qcrypto_block_encrypt(s->crypto, sector_num *
> BDRV_SECTOR_SIZE,
> - buf, n * BDRV_SECTOR_SIZE, NULL) < 0) {
> +if (qcrypto_block_encrypt(s->crypto, offset, buf, n, NULL) < 0) {
> ret = -EIO;
> break;
> }
> }
>
> hd_iov.iov_base = (void *)buf;
> -hd_iov.iov_len = n * 512;
> +hd_iov.iov_len = n;
> qemu_iovec_init_external(_qiov, _iov, 1);
> qemu_co_mutex_unlock(>lock);
> BLKDBG_EVENT(bs->file, BLKDBG_WRITE_AIO);
> -ret = bdrv_co_writev(bs->file,
> - (cluster_offset >> 9) + index_in_cluster,
> - n, _qiov);
> +ret = bdrv_co_pwritev(bs->file, cluster_offset + offset_in_cluster,
> + n, _qiov, 0);
> qemu_co_mutex_lock(>lock);
> if (ret < 0) {
> break;
> }
> ret = 0;
>
> -nb_sectors -= n;
> -sector_num += n;
> -buf += n * 512;
> +bytes -= n;
> +offset += n;
> +buf += n;
> }
> qemu_co_mutex_unlock(>lock);
>
> --
> 2.14.3
>
>
Reviewed-by: Jeff Cody
Re: [Qemu-devel] [Qemu-block] [PATCH v2 3/8] qcow: Switch qcow_co_readv to byte-based calls
On Thu, May 31, 2018 at 03:50:41PM -0500, Eric Blake wrote:
> We are gradually moving away from sector-based interfaces, towards
> byte-based. Make the change for the internals of the qcow
> driver read function, by iterating over offset/bytes instead of
> sector_num/nb_sectors, and with a rename of index_in_cluster and
> repurposing of n to track bytes instead of sectors.
>
> A later patch will then switch the qcow driver as a whole over
> to byte-based operation.
>
> Signed-off-by: Eric Blake
>
> ---
> v2: prefer 64-bit * over 32-bit <<, rename variable for legibility [Kevin]
> ---
> block/qcow.c | 42 --
> 1 file changed, 20 insertions(+), 22 deletions(-)
>
> diff --git a/block/qcow.c b/block/qcow.c
> index 42d1bbb7643..b90915218ff 100644
> --- a/block/qcow.c
> +++ b/block/qcow.c
> @@ -617,13 +617,15 @@ static coroutine_fn int qcow_co_readv(BlockDriverState
> *bs, int64_t sector_num,
> int nb_sectors, QEMUIOVector *qiov)
> {
> BDRVQcowState *s = bs->opaque;
> -int index_in_cluster;
> +int offset_in_cluster;
> int ret = 0, n;
> uint64_t cluster_offset;
> struct iovec hd_iov;
> QEMUIOVector hd_qiov;
> uint8_t *buf;
> void *orig_buf;
> +int64_t offset = sector_num * BDRV_SECTOR_SIZE;
> +int64_t bytes = nb_sectors * BDRV_SECTOR_SIZE;
>
> if (qiov->niov > 1) {
> buf = orig_buf = qemu_try_blockalign(bs, qiov->size);
> @@ -637,36 +639,35 @@ static coroutine_fn int qcow_co_readv(BlockDriverState
> *bs, int64_t sector_num,
>
> qemu_co_mutex_lock(>lock);
>
> -while (nb_sectors != 0) {
> +while (bytes != 0) {
> /* prepare next request */
> -ret = get_cluster_offset(bs, sector_num << 9,
> - 0, 0, 0, 0, _offset);
> +ret = get_cluster_offset(bs, offset, 0, 0, 0, 0, _offset);
> if (ret < 0) {
> break;
> }
> -index_in_cluster = sector_num & (s->cluster_sectors - 1);
> -n = s->cluster_sectors - index_in_cluster;
> -if (n > nb_sectors) {
> -n = nb_sectors;
> +offset_in_cluster = offset & (s->cluster_size - 1);
> +n = s->cluster_size - offset_in_cluster;
> +if (n > bytes) {
> +n = bytes;
> }
>
> if (!cluster_offset) {
> if (bs->backing) {
> /* read from the base image */
> hd_iov.iov_base = (void *)buf;
> -hd_iov.iov_len = n * 512;
> +hd_iov.iov_len = n;
> qemu_iovec_init_external(_qiov, _iov, 1);
> qemu_co_mutex_unlock(>lock);
> /* qcow2 emits this on bs->file instead of bs->backing */
> BLKDBG_EVENT(bs->file, BLKDBG_READ_BACKING_AIO);
> -ret = bdrv_co_readv(bs->backing, sector_num, n, _qiov);
> +ret = bdrv_co_preadv(bs->backing, offset, n, _qiov, 0);
> qemu_co_mutex_lock(>lock);
> if (ret < 0) {
> break;
> }
> } else {
> /* Note: in this case, no need to wait */
> -memset(buf, 0, 512 * n);
> +memset(buf, 0, n);
> }
> } else if (cluster_offset & QCOW_OFLAG_COMPRESSED) {
> /* add AIO support for compressed blocks ? */
> @@ -674,21 +675,19 @@ static coroutine_fn int qcow_co_readv(BlockDriverState
> *bs, int64_t sector_num,
> ret = -EIO;
> break;
> }
> -memcpy(buf,
> - s->cluster_cache + index_in_cluster * 512, 512 * n);
> +memcpy(buf, s->cluster_cache + offset_in_cluster, n);
> } else {
> if ((cluster_offset & 511) != 0) {
> ret = -EIO;
> break;
> }
> hd_iov.iov_base = (void *)buf;
> -hd_iov.iov_len = n * 512;
> +hd_iov.iov_len = n;
> qemu_iovec_init_external(_qiov, _iov, 1);
> qemu_co_mutex_unlock(>lock);
> BLKDBG_EVENT(bs->file, BLKDBG_READ_AIO);
> -ret = bdrv_co_readv(bs->file,
> -(cluster_offset >> 9) + index_in_cluster,
> -n, _qiov);
> +ret = bdrv_co_preadv(bs->file, cluster_offset +
> offset_in_cluster,
> + n, _qiov, 0);
> qemu_co_mutex_lock(>lock);
> if (ret < 0) {
> break;
> @@ -696,8 +695,7 @@ static coroutine_fn int qcow_co_readv(BlockDriverState
> *bs, int64_t sector_num,
> if (bs->encrypted) {
> assert(s->crypto);
> if (qcrypto_block_decrypt(s->crypto,
> - sector_num * BDRV_SECTOR_SIZE, buf,
> - n * BDRV_SECTOR_SIZE,
Re: [Qemu-devel] [PATCH] tcg-target.inc.c: Use byte form of xgetbv instruction
Hi,
This series seems to have some coding style problems. See output below for
more information:
Type: series
Message-id: 20180604211106.4442-1-programmingk...@gmail.com
Subject: [Qemu-devel] [PATCH] tcg-target.inc.c: Use byte form of xgetbv
instruction
=== TEST SCRIPT BEGIN ===
#!/bin/bash
BASE=base
n=1
total=$(git log --oneline $BASE.. | wc -l)
failed=0
git config --local diff.renamelimit 0
git config --local diff.renames True
git config --local diff.algorithm histogram
commits="$(git log --format=%H --reverse $BASE..)"
for c in $commits; do
echo "Checking PATCH $n/$total: $(git log -n 1 --format=%s $c)..."
if ! git show $c --format=email | ./scripts/checkpatch.pl --mailback -; then
failed=1
echo
fi
n=$((n+1))
done
exit $failed
=== TEST SCRIPT END ===
Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
From https://github.com/patchew-project/qemu
t [tag update]patchew/20180531205046.153256-1-ebl...@redhat.com ->
patchew/20180531205046.153256-1-ebl...@redhat.com
* [new tag]
patchew/20180604211106.4442-1-programmingk...@gmail.com ->
patchew/20180604211106.4442-1-programmingk...@gmail.com
Switched to a new branch 'test'
8ee6e3972e tcg-target.inc.c: Use byte form of xgetbv instruction
=== OUTPUT BEGIN ===
Checking PATCH 1/1: tcg-target.inc.c: Use byte form of xgetbv instruction...
ERROR: trailing whitespace
#23: FILE: tcg/i386/tcg-target.inc.c:3505:
+ * The xgetbv instruction is not available to older versions of
the $
ERROR: spaces required around that ':' (ctx:VxW)
#26: FILE: tcg/i386/tcg-target.inc.c:3508:
+asm (".byte 0x0f, 0x01, 0xd0": "=a" (xcrl), "=d" (xcrh) : "c" (0));
^
total: 2 errors, 0 warnings, 12 lines checked
Your patch has style problems, please review. If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.
=== OUTPUT END ===
Test command exited with code: 1
---
Email generated automatically by Patchew [http://patchew.org/].
Please send your feedback to patchew-de...@redhat.com
Re: [Qemu-devel] [PATCH 1/2] i386: define the AMD 'amd-ssbd' CPUID feature bit
On Mon, Jun 04, 2018 at 04:22:05PM -0400, Konrad Rzeszutek Wilk wrote: > On Mon, Jun 04, 2018 at 05:07:01PM -0300, Eduardo Habkost wrote: > > On Fri, Jun 01, 2018 at 11:38:08AM -0400, Konrad Rzeszutek Wilk wrote: > > > AMD future CPUs expose _two_ ways to utilize the Intel equivalant > > > of the Speculative Store Bypass Disable. The first is via > > > the virtualized VIRT_SPEC CTRL MSR (0xC001_011f) and the second > > > is via the SPEC_CTRL MSR (0x48). The document titled: > > > 124441_AMD64_SpeculativeStoreBypassDisable_Whitepaper_final.pdf > > > > > > gives priority of SPEC CTRL MSR over the VIRT SPEC CTRL MSR. > > > > > > A copy of this document is available at > > > https://bugzilla.kernel.org/show_bug.cgi?id=199889 > > > > > > Anyhow, this means that on future AMD CPUs there will be _two_ ways to > > > deal with SSBD. > > > > Does anybody know if there are AMD CPUs where virt-ssbd won't > > work and would require amd-ssbd to mitigate vulnerabilities? > > > > Also, do we have kernel arch/x86/kvm/cpuid.c patches, already? > > Not yet. They are being discussed right now. I figured I would send > these patches out as a 'Hey, coming at you!', but failed to change > the title to be 'RFC'. OK. I was queueing them on x86-next, but I'm going drop them by now. > > > I prefer to add new CPUID flag names only after the flag name is > > already agreed upon on the kernel side. > > Of course. I will respin once that discussion has calmed down. Thanks! BTW, it looks like the patch on LKML[1] will make bit 26 appear on /proc/cpuinfo as "amd_ssb_no", is that correct? If that's the case, I'd prefer to make the QEMU flag to match the name used by Linux, and be called "amd-ssb-no" (which sounds weird to me, but at least it will be consistent with /proc/cpuinfo). [1] https://patchwork.kernel.org/patch/10443689/ -- Eduardo
[Qemu-devel] [PATCH] tcg-target.inc.c: Use byte form of xgetbv instruction
The assembler in most versions of Mac OS X is pretty old and does not support
the xgetbv instruction. To go around this problem the raw encoding of the
instruction is used instead.
Signed-off-by: John Arbuckle
---
tcg/i386/tcg-target.inc.c | 6 +-
1 file changed, 5 insertions(+), 1 deletion(-)
diff --git a/tcg/i386/tcg-target.inc.c b/tcg/i386/tcg-target.inc.c
index 5357909..82e004a 100644
--- a/tcg/i386/tcg-target.inc.c
+++ b/tcg/i386/tcg-target.inc.c
@@ -3501,7 +3501,11 @@ static void tcg_target_init(TCGContext *s)
sure of not hitting invalid opcode. */
if (c & bit_OSXSAVE) {
unsigned xcrl, xcrh;
-asm ("xgetbv" : "=a" (xcrl), "=d" (xcrh) : "c" (0));
+/*
+ * The xgetbv instruction is not available to older versions of
the
+ * assembler, so we encode the instruction manually.
+ */
+asm (".byte 0x0f, 0x01, 0xd0": "=a" (xcrl), "=d" (xcrh) : "c" (0));
if ((xcrl & 6) == 6) {
have_avx1 = (c & bit_AVX) != 0;
have_avx2 = (b7 & bit_AVX2) != 0;
--
2.10.2
Re: [Qemu-devel] [PATCH RFC] hw/pc: set q35 as the default x86 machine
On Mon, Jun 04, 2018 at 09:40:15PM +0300, Marcel Apfelbaum wrote: > > > On 06/04/2018 07:56 PM, Daniel P. Berrangé wrote: > > On Mon, Jun 04, 2018 at 07:48:51PM +0300, Michael S. Tsirkin wrote: > > > On Mon, Jun 04, 2018 at 02:01:29PM +0100, Daniel P. Berrangé wrote: > > > > On Mon, Jun 04, 2018 at 09:54:15AM -0300, Eduardo Habkost wrote: > > > > > On Mon, Jun 04, 2018 at 04:38:22AM +0300, Michael S. Tsirkin wrote: > > > > > > On Sun, Jun 03, 2018 at 12:27:49PM +0300, Marcel Apfelbaum wrote: > > > > > > > Moving to QEMU 3.0 seems like a good opportunity for such a > > > > > > > change. > > > > > > > > > > > > > > I440FX is really old and does not support modern features like > > > > > > > IOMMU. > > > > > > > Q35's SATA emulation is faster than pc's IDE, native PCI express > > > > > > > hotplug > > > > > > > is cleaner than ACPI based one and so on... > > > > > > > > > > > > > > Also the libvirt guys added very good support for the Q35 machine > > > > > > > (thanks!). > > > > > > > > > > > > > > Management software should always specify the machine type and > > > > > > > for the > > > > > > > current setups, adding '-machine pc' to the command line is not > > > > > > > such a > > > > > > > big deal. > > > > > > > > > > > > > > In time the pc machine will fade out and we will probably stop > > > > > > > adding > > > > > > > new versions at some point. > > > > > > > > > > > > > > Signed-off-by: Marcel Apfelbaum > > > > > > For command line users, I think changing the default isn't nice. > > > > > > > > > > > > Yes it's easy to add -machine pc but there's no documentation > > > > > > that tells you to do so. Add to that shortcuts like -cdrom > > > > > > stop working, hotplug needs extra bridges to work, and one > > > > > > can see that while management tool users benefit from q35, > > > > > > command line users will suffer. > > > > > > > > > > > > Can't we add a tag for management without changing the command line > > > > > > default? How about "management-default"? "recommended"? "latest"? > > > > > We could add new aliases if they are useful for management > > > > > software, but we would need a well-defined use case and set of > > > > > requirements+expectations for the new alias. > > > > I'm not convinced by the idea of adding a distinct default "for mgmt". > > > > All > > > > the problems described wrt 'q35' vs 'pc' apply equally to management > > > > apps > > > > as they do to humans. It just happens that one common mgmt layer > > > > (libvirt) > > > > knows how to handle some of the complexity of q35. Other mgmt apps > > > > though > > > > are just as likely to be hurt by the change as humans are. So > > > > effectively > > > > the proposed "for mgmt" is actually "for libvirt >= some version", > > > > which > > > > feels like a layering violation to me. > > > Is libvirt happy to just hard-code q35 for now then? > > I'm pretty wary of doing that, as I feel 'pc' has broader OS compatibility > > than 'q35', so we'd be likely to cause breakage for users. > > > > IMHO, defaults are something better expressed in libosinfo, so if there's > > guests where we think q35 is a better choice, we can record it there and > > leave everything else on pc to avoid risk of breakage. > > The only info we need to pass properly to management systems is: > "Use q35 unless your guests are really old". Or if your existing disk image contains drivers or other software that work only with pc. Or if it's going to trigger Windows license reactivation on a disk image prepared using pc. The stack can't change the default without breaking existing guest images. Whether it's a reasonable risk or it's unacceptable depends on what are the intended audience and use cases of a given management stack. I don't think QEMU or libvirt can make that decision for them. That said, if something is going to break as soon as the default in QEMU changes, that's a bug in the management stack. Management stacks should stop assuming the default machine-type in libvirt+QEMU is never going to change. -- Eduardo
Re: [Qemu-devel] [PATCH 10/33] linux-user: Split out brk, close, exit, read, write
On 06/04/2018 01:17 PM, Laurent Vivier wrote:
> Le 01/06/2018 à 09:30, Richard Henderson a écrit :
>> These are relatively simple unconditionally defined syscalls.
>>
>> Signed-off-by: Richard Henderson
>> ---
>> linux-user/syscall.c | 198 ---
>> 1 file changed, 111 insertions(+), 87 deletions(-)
>>
>> diff --git a/linux-user/syscall.c b/linux-user/syscall.c
>> index fc3dc3f40d..b0d268dab7 100644
>> --- a/linux-user/syscall.c
>> +++ b/linux-user/syscall.c
>> @@ -7984,6 +7984,112 @@ IMPL(enosys)
>> return do_unimplemented(num);
>> }
>>
>> +IMPL(brk)
>> +{
>> +return do_brk(arg1);
>> +}
>> +
>> +IMPL(close)
>> +{
>> +if (is_hostfd(arg1)) {
>> +return -TARGET_EBADF;
>> +}
>> +fd_trans_unregister(arg1);
>> +return get_errno(close(arg1));
>> +}
>> +
>> +IMPL(exit)
>> +{
>> +CPUState *cpu = ENV_GET_CPU(cpu_env);
>> +
>> +/* In old applications this may be used to implement _exit(2).
>> + However in threaded applictions it is used for thread termination,
>> + and _exit_group is used for application termination.
>> + Do thread termination if we have more then one thread. */
>> +if (block_signals()) {
>> +return -TARGET_ERESTARTSYS;
>> +}
>> +
>> +cpu_list_lock();
>> +
>> +if (CPU_NEXT(first_cpu)) {
>> +/* Remove the CPU from the list. */
>> +QTAILQ_REMOVE(, cpu, node);
>> +cpu_list_unlock();
>> +
>> +TaskState *ts = cpu->opaque;
>> +if (ts->child_tidptr) {
>> +put_user_u32(0, ts->child_tidptr);
>> +sys_futex(g2h(ts->child_tidptr), FUTEX_WAKE, INT_MAX,
>> + NULL, NULL, 0);
>> +}
>> +thread_cpu = NULL;
>> +object_unref(OBJECT(cpu));
>> +g_free(ts);
>> +rcu_unregister_thread();
>> +pthread_exit(NULL);
>> +} else {
>> +cpu_list_unlock();
>> +
>> +#ifdef TARGET_GPROF
>> +_mcleanup();
>> +#endif
>> +gdb_exit(cpu_env, arg1);
>> +_exit(arg1);
>> +}
>> +g_assert_not_reached();
>> +}
>> +
>> +IMPL(read)
>> +{
>> +abi_long ret;
>> +char *fn;
>> +
>> +if (arg3 == 0) {
>> +return 0;
>> +}
>> +if (is_hostfd(arg1)) {
>> +return -TARGET_EBADF;
>> +}
>> +fn = lock_user(VERIFY_WRITE, arg2, arg3, 0);
>> +if (!fn) {
>> +return -TARGET_EFAULT;
>> +}
>> +ret = get_errno(safe_read(arg1, fn, arg3));
>> +if (ret >= 0 && fd_trans_host_to_target_data(arg1)) {
>> +ret = fd_trans_host_to_target_data(arg1)(fn, ret);
>> +}
>> +unlock_user(fn, arg2, ret);
>> +return ret;
>> +}
>> +
>> +IMPL(write)
>> +{
>> +abi_long ret;
>> +char *fn;
>> +
>> +if (is_hostfd(arg1)) {
>> +return -TARGET_EBADF;
>> +}
>> +fn = lock_user(VERIFY_READ, arg2, arg3, 1);
>> +if (!fn) {
>> +return -TARGET_EFAULT;
>> +}
>> +if (fd_trans_target_to_host_data(arg1)) {
>> +void *copy = g_malloc(arg3);
>> +memcpy(copy, fn, arg3);
>> +ret = fd_trans_target_to_host_data(arg1)(copy, arg3);
>> +if (ret >= 0) {
>> +ret = get_errno(safe_write(arg1, copy, ret));
>> +}
>> +g_free(copy);
>> +} else {
>> +ret = get_errno(safe_write(arg1, fn, arg3));
>> +}
>> +unlock_user(fn, arg2, ret);
>> +return ret;
>> +}
>> +
>> /* This is an internal helper for do_syscall so that it is easier
>> * to have a single return point, so that actions, such as logging
>> * of syscall results, can be performed.
>> @@ -7999,83 +8105,6 @@ IMPL(everything_else)
>> char *fn;
>>
>> switch(num) {
>> -case TARGET_NR_exit:
>> -/* In old applications this may be used to implement _exit(2).
>> - However in threaded applictions it is used for thread
>> termination,
>> - and _exit_group is used for application termination.
>> - Do thread termination if we have more then one thread. */
>> -
>> -if (block_signals()) {
>> -return -TARGET_ERESTARTSYS;
>> -}
>> -
>> -cpu_list_lock();
>> -
>> -if (CPU_NEXT(first_cpu)) {
>> -TaskState *ts;
>> -
>> -/* Remove the CPU from the list. */
>> -QTAILQ_REMOVE(, cpu, node);
>> -
>> -cpu_list_unlock();
>> -
>> -ts = cpu->opaque;
>> -if (ts->child_tidptr) {
>> -put_user_u32(0, ts->child_tidptr);
>> -sys_futex(g2h(ts->child_tidptr), FUTEX_WAKE, INT_MAX,
>> - NULL, NULL, 0);
>> -}
>> -thread_cpu = NULL;
>> -object_unref(OBJECT(cpu));
>> -g_free(ts);
>> -rcu_unregister_thread();
>> -pthread_exit(NULL);
>> -}
>> -
>> -cpu_list_unlock();
>> -#ifdef TARGET_GPROF
>> -_mcleanup();
>> -#endif
>> -gdb_exit(cpu_env, arg1);
>> -_exit(arg1);
Re: [Qemu-devel] [Qemu-block] [PATCH v2 2/8] qcow: Switch get_cluster_offset to be byte-based
On Thu, May 31, 2018 at 03:50:40PM -0500, Eric Blake wrote:
> We are gradually moving away from sector-based interfaces, towards
> byte-based. Make the change for the internal helper function
> get_cluster_offset(), by changing n_start and n_end to by byte
s/by\ byte/be\ byte/
> offsets rather than sector indices within the cluster being
> allocated. However, assert that these values are still
> sector-aligned (at least qcrypto_block_encrypt() still wants that).
> For now we get that alignment for free because we still use
> sector-based driver callbacks.
>
> A later patch will then switch the qcow driver as a whole over
> to byte-based operation; but will still leave things at sector
> alignments as it is not worth auditing the qcow image format
> to worry about sub-sector requests.
>
> Signed-off-by: Eric Blake
Reviewed-by: Jeff Cody
>
> ---
> v2: assert sector alignment [Max]
> ---
> block/qcow.c | 29 +++--
> 1 file changed, 15 insertions(+), 14 deletions(-)
>
> diff --git a/block/qcow.c b/block/qcow.c
> index 3ba2ca25ea5..42d1bbb7643 100644
> --- a/block/qcow.c
> +++ b/block/qcow.c
> @@ -345,8 +345,8 @@ static int qcow_reopen_prepare(BDRVReopenState *state,
> *
> * 0 to not allocate.
> *
> - * 1 to allocate a normal cluster (for sector indexes 'n_start' to
> - * 'n_end')
> + * 1 to allocate a normal cluster (for sector-aligned byte offsets 'n_start'
> + * to 'n_end' within the cluster)
> *
> * 2 to allocate a compressed cluster of size
> * 'compressed_size'. 'compressed_size' must be > 0 and <
> @@ -440,9 +440,10 @@ static int get_cluster_offset(BlockDriverState *bs,
> if (!allocate)
> return 0;
> BLKDBG_EVENT(bs->file, BLKDBG_CLUSTER_ALLOC);
> +assert(QEMU_IS_ALIGNED(n_start | n_end, BDRV_SECTOR_SIZE));
> /* allocate a new cluster */
> if ((cluster_offset & QCOW_OFLAG_COMPRESSED) &&
> -(n_end - n_start) < s->cluster_sectors) {
> +(n_end - n_start) < s->cluster_size) {
> /* if the cluster is already compressed, we must
> decompress it in the case it is not completely
> overwritten */
> @@ -480,16 +481,15 @@ static int get_cluster_offset(BlockDriverState *bs,
> /* if encrypted, we must initialize the cluster
> content which won't be written */
> if (bs->encrypted &&
> -(n_end - n_start) < s->cluster_sectors) {
> -uint64_t start_sect;
> +(n_end - n_start) < s->cluster_size) {
> +uint64_t start_offset;
> assert(s->crypto);
> -start_sect = (offset & ~(s->cluster_size - 1)) >> 9;
> -for(i = 0; i < s->cluster_sectors; i++) {
> +start_offset = offset & ~(s->cluster_size - 1);
> +for (i = 0; i < s->cluster_size; i += BDRV_SECTOR_SIZE) {
> if (i < n_start || i >= n_end) {
> -memset(s->cluster_data, 0x00, 512);
> +memset(s->cluster_data, 0x00, BDRV_SECTOR_SIZE);
> if (qcrypto_block_encrypt(s->crypto,
> - (start_sect + i) *
> - BDRV_SECTOR_SIZE,
> + start_offset + i,
>s->cluster_data,
>BDRV_SECTOR_SIZE,
>NULL) < 0) {
> @@ -497,8 +497,9 @@ static int get_cluster_offset(BlockDriverState *bs,
> }
> BLKDBG_EVENT(bs->file, BLKDBG_WRITE_AIO);
> ret = bdrv_pwrite(bs->file,
> - cluster_offset + i * 512,
> - s->cluster_data, 512);
> + cluster_offset + i,
> + s->cluster_data,
> + BDRV_SECTOR_SIZE);
> if (ret < 0) {
> return ret;
> }
> @@ -758,8 +759,8 @@ static coroutine_fn int qcow_co_writev(BlockDriverState
> *bs, int64_t sector_num,
> n = nb_sectors;
> }
> ret = get_cluster_offset(bs, sector_num << 9, 1, 0,
> - index_in_cluster,
> - index_in_cluster + n, _offset);
> + index_in_cluster << 9,
> + (index_in_cluster + n) << 9,
> _offset);
> if (ret < 0) {
> break;
> }
> --
> 2.14.3
>
>
Re: [Qemu-devel] Preconfig state reachable without --preconfig given
On Mon, 4 Jun 2018 16:04:33 +0200 Max Reitz wrote: > On 2018-06-02 12:46, Michal Privoznik wrote: > > On 06/01/2018 03:28 PM, Max Reitz wrote: > >> Hi, > >> > >> The @preconfig RunState documentation states: > >> > >>> The state is reachable only if the --preconfig CLI option is used. > >> > >> However, this is not true: > >> > >> $ echo | x86_64-softmmu/qemu-system-x86_64 -monitor stdio > >> QEMU 2.12.50 monitor - type 'help' for more information > >> (qemu) > >> HMP not available in preconfig state, use QMP instead > > > > Not sure if this is the same bug, but I've noticed libvirt having > > troubles detecting capabilities of qemu and debugging lead to this patch: > > > > http://lists.nongnu.org/archive/html/qemu-devel/2018-06/msg00367.html > > > > (which by no means I claim is proper solution. It might be viewed as > > workaround by expert qemu devels) > > I haven't investigated further, but that patch breaks iotest 091 (which > tests migration). It just stops now after the migration has started, so > it never completes. > > Actually, it seems to break all migration iotests (but 203, which > migrates to /dev/null), so I suspect it breaks migration as a whole. Manual migration to file and restore from it, was tested manually and worked fine, migration-test from 'make check' is ok as well. It's probably due broken to "echo foo | $QEMU" There is another issue with -nodefaults option leading to indefinite timeout even if --preconfig wasn't used. I've just posted fix for that so that it won't be masked out by Michal/Daniel's fix. But I've run iotests and -nodefaults fix doesn't affect outcome of the tests (the same 4 failures). > > Max >
Re: [Qemu-devel] [PATCH 1/2] i386: define the AMD 'amd-ssbd' CPUID feature bit
On Mon, Jun 04, 2018 at 05:07:01PM -0300, Eduardo Habkost wrote:
> On Fri, Jun 01, 2018 at 11:38:08AM -0400, Konrad Rzeszutek Wilk wrote:
> > AMD future CPUs expose _two_ ways to utilize the Intel equivalant
> > of the Speculative Store Bypass Disable. The first is via
> > the virtualized VIRT_SPEC CTRL MSR (0xC001_011f) and the second
> > is via the SPEC_CTRL MSR (0x48). The document titled:
> > 124441_AMD64_SpeculativeStoreBypassDisable_Whitepaper_final.pdf
> >
> > gives priority of SPEC CTRL MSR over the VIRT SPEC CTRL MSR.
> >
> > A copy of this document is available at
> > https://bugzilla.kernel.org/show_bug.cgi?id=199889
> >
> > Anyhow, this means that on future AMD CPUs there will be _two_ ways to
> > deal with SSBD.
>
> Does anybody know if there are AMD CPUs where virt-ssbd won't
> work and would require amd-ssbd to mitigate vulnerabilities?
>
> Also, do we have kernel arch/x86/kvm/cpuid.c patches, already?
Not yet. They are being discussed right now. I figured I would send
these patches out as a 'Hey, coming at you!', but failed to change
the title to be 'RFC'.
> I prefer to add new CPUID flag names only after the flag name is
> already agreed upon on the kernel side.
Of course. I will respin once that discussion has calmed down.
>
>
> >
> > Signed-off-by: Konrad Rzeszutek Wilk
> > ---
> > target/i386/cpu.c | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/target/i386/cpu.c b/target/i386/cpu.c
> > index 52d334a..f91990c 100644
> > --- a/target/i386/cpu.c
> > +++ b/target/i386/cpu.c
> > @@ -490,7 +490,7 @@ static FeatureWordInfo feature_word_info[FEATURE_WORDS]
> > = {
> > "ibpb", NULL, NULL, NULL,
> > NULL, NULL, NULL, NULL,
> > NULL, NULL, NULL, NULL,
> > -NULL, "virt-ssbd", NULL, NULL,
> > +"amd-ssbd", "virt-ssbd", NULL, NULL,
> > NULL, NULL, NULL, NULL,
> > },
> > .cpuid_eax = 0x8008,
> > --
> > 1.8.3.1
> >
> >
>
> --
> Eduardo
Re: [Qemu-devel] [PATCH 1/2] i386: define the AMD 'amd-ssbd' CPUID feature bit
On Mon, Jun 04, 2018 at 09:54:40AM +0100, Daniel P. Berrangé wrote: > On Fri, Jun 01, 2018 at 11:38:08AM -0400, Konrad Rzeszutek Wilk wrote: > > AMD future CPUs expose _two_ ways to utilize the Intel equivalant > > of the Speculative Store Bypass Disable. The first is via > > the virtualized VIRT_SPEC CTRL MSR (0xC001_011f) and the second > > is via the SPEC_CTRL MSR (0x48). The document titled: > > 124441_AMD64_SpeculativeStoreBypassDisable_Whitepaper_final.pdf > > > > gives priority of SPEC CTRL MSR over the VIRT SPEC CTRL MSR. > > > > A copy of this document is available at > > https://bugzilla.kernel.org/show_bug.cgi?id=199889 > > > > Anyhow, this means that on future AMD CPUs there will be _two_ ways to > > deal with SSBD. > > Oh what fun ;-) > > Unless I'm mistaken the current Linux kernel doesn't know about these > new amd-ssbd / amd-no-ssb flags either. Will you also be sending patches > for that half of the problem ? I sent them as well. But forgot to CC qemu-devel :-(
Re: [Qemu-devel] [PATCH 10/33] linux-user: Split out brk, close, exit, read, write
Le 01/06/2018 à 09:30, Richard Henderson a écrit :
> These are relatively simple unconditionally defined syscalls.
>
> Signed-off-by: Richard Henderson
> ---
> linux-user/syscall.c | 198 ---
> 1 file changed, 111 insertions(+), 87 deletions(-)
>
> diff --git a/linux-user/syscall.c b/linux-user/syscall.c
> index fc3dc3f40d..b0d268dab7 100644
> --- a/linux-user/syscall.c
> +++ b/linux-user/syscall.c
> @@ -7984,6 +7984,112 @@ IMPL(enosys)
> return do_unimplemented(num);
> }
>
> +IMPL(brk)
> +{
> +return do_brk(arg1);
> +}
> +
> +IMPL(close)
> +{
> +if (is_hostfd(arg1)) {
> +return -TARGET_EBADF;
> +}
> +fd_trans_unregister(arg1);
> +return get_errno(close(arg1));
> +}
> +
> +IMPL(exit)
> +{
> +CPUState *cpu = ENV_GET_CPU(cpu_env);
> +
> +/* In old applications this may be used to implement _exit(2).
> + However in threaded applictions it is used for thread termination,
> + and _exit_group is used for application termination.
> + Do thread termination if we have more then one thread. */
> +if (block_signals()) {
> +return -TARGET_ERESTARTSYS;
> +}
> +
> +cpu_list_lock();
> +
> +if (CPU_NEXT(first_cpu)) {
> +/* Remove the CPU from the list. */
> +QTAILQ_REMOVE(, cpu, node);
> +cpu_list_unlock();
> +
> +TaskState *ts = cpu->opaque;
> +if (ts->child_tidptr) {
> +put_user_u32(0, ts->child_tidptr);
> +sys_futex(g2h(ts->child_tidptr), FUTEX_WAKE, INT_MAX,
> + NULL, NULL, 0);
> +}
> +thread_cpu = NULL;
> +object_unref(OBJECT(cpu));
> +g_free(ts);
> +rcu_unregister_thread();
> +pthread_exit(NULL);
> +} else {
> +cpu_list_unlock();
> +
> +#ifdef TARGET_GPROF
> +_mcleanup();
> +#endif
> +gdb_exit(cpu_env, arg1);
> +_exit(arg1);
> +}
> +g_assert_not_reached();
> +}
> +
> +IMPL(read)
> +{
> +abi_long ret;
> +char *fn;
> +
> +if (arg3 == 0) {
> +return 0;
> +}
> +if (is_hostfd(arg1)) {
> +return -TARGET_EBADF;
> +}
> +fn = lock_user(VERIFY_WRITE, arg2, arg3, 0);
> +if (!fn) {
> +return -TARGET_EFAULT;
> +}
> +ret = get_errno(safe_read(arg1, fn, arg3));
> +if (ret >= 0 && fd_trans_host_to_target_data(arg1)) {
> +ret = fd_trans_host_to_target_data(arg1)(fn, ret);
> +}
> +unlock_user(fn, arg2, ret);
> +return ret;
> +}
> +
> +IMPL(write)
> +{
> +abi_long ret;
> +char *fn;
> +
> +if (is_hostfd(arg1)) {
> +return -TARGET_EBADF;
> +}
> +fn = lock_user(VERIFY_READ, arg2, arg3, 1);
> +if (!fn) {
> +return -TARGET_EFAULT;
> +}
> +if (fd_trans_target_to_host_data(arg1)) {
> +void *copy = g_malloc(arg3);
> +memcpy(copy, fn, arg3);
> +ret = fd_trans_target_to_host_data(arg1)(copy, arg3);
> +if (ret >= 0) {
> +ret = get_errno(safe_write(arg1, copy, ret));
> +}
> +g_free(copy);
> +} else {
> +ret = get_errno(safe_write(arg1, fn, arg3));
> +}
> +unlock_user(fn, arg2, ret);
> +return ret;
> +}
> +
> /* This is an internal helper for do_syscall so that it is easier
> * to have a single return point, so that actions, such as logging
> * of syscall results, can be performed.
> @@ -7999,83 +8105,6 @@ IMPL(everything_else)
> char *fn;
>
> switch(num) {
> -case TARGET_NR_exit:
> -/* In old applications this may be used to implement _exit(2).
> - However in threaded applictions it is used for thread termination,
> - and _exit_group is used for application termination.
> - Do thread termination if we have more then one thread. */
> -
> -if (block_signals()) {
> -return -TARGET_ERESTARTSYS;
> -}
> -
> -cpu_list_lock();
> -
> -if (CPU_NEXT(first_cpu)) {
> -TaskState *ts;
> -
> -/* Remove the CPU from the list. */
> -QTAILQ_REMOVE(, cpu, node);
> -
> -cpu_list_unlock();
> -
> -ts = cpu->opaque;
> -if (ts->child_tidptr) {
> -put_user_u32(0, ts->child_tidptr);
> -sys_futex(g2h(ts->child_tidptr), FUTEX_WAKE, INT_MAX,
> - NULL, NULL, 0);
> -}
> -thread_cpu = NULL;
> -object_unref(OBJECT(cpu));
> -g_free(ts);
> -rcu_unregister_thread();
> -pthread_exit(NULL);
> -}
> -
> -cpu_list_unlock();
> -#ifdef TARGET_GPROF
> -_mcleanup();
> -#endif
> -gdb_exit(cpu_env, arg1);
> -_exit(arg1);
> -return 0; /* avoid warning */
> -case TARGET_NR_read:
> -if (arg3 == 0) {
> -return 0;
> -} else {
> -if (is_hostfd(arg1)) {
> -return
[Qemu-devel] [PATCH] vl.c: make default main_loop_wait() timeout independed of slirp
Since commit (047f7038f58 cli: add --preconfig option) QEMU is
stuck with indefinite timeout in os_host_main_loop_wait()
at RUN_STATE_PRECONFIG even if --preconfig option wasn't used
when it's started with -nodefaults CLI option like this:
./s390x-softmmu/qemu-system-s390x -nodefaults
It's caused by the fact that slirp_pollfds_fill() bails out early
and slirp_update_timeout() won't be called to update timeout
to a reasonable value (1 sec) so timeout would be left with
infinite value (0x).
Default infinite timeout though doen't make sense and reducing
it to 1 second as in slirp_update_timeout() won't affect host.
Fix issue by simplifying default timeout to the same 1sec as it
is in slirp_update_timeout() and cleanup the later. It makes
default timeout the same regardless of slirp_pollfds_fill()
exited early or not (i.e. -nodefaults won't have any effect on
main_loop_wait() anymore, which would provide more consistent
behavior between both variants of startup).
Reported-by: Lukáš Doktor
Signed-off-by: Igor Mammedov
---
PS:
it doesn't fix issue reported by Max where
"echo foo | $QEMU"
is also broken due to commit 047f7038f58, but there is antoher fix
on the list to fix that (either Michal's or Daniel's).
---
slirp/slirp.c| 10 ++
util/main-loop.c | 13 ++---
2 files changed, 4 insertions(+), 19 deletions(-)
diff --git a/slirp/slirp.c b/slirp/slirp.c
index 1cb6b07..1112f86 100644
--- a/slirp/slirp.c
+++ b/slirp/slirp.c
@@ -358,13 +358,8 @@ void slirp_cleanup(Slirp *slirp)
static void slirp_update_timeout(uint32_t *timeout)
{
Slirp *slirp;
-uint32_t t;
-if (*timeout <= TIMEOUT_FAST) {
-return;
-}
-
-t = MIN(1000, *timeout);
+assert(*timeout > TIMEOUT_FAST);
/* If we have tcp timeout with slirp, then we will fill @timeout with
* more precise value.
@@ -375,10 +370,9 @@ static void slirp_update_timeout(uint32_t *timeout)
return;
}
if (slirp->do_slowtimo) {
-t = MIN(TIMEOUT_SLOW, t);
+*timeout = MIN(TIMEOUT_SLOW, *timeout);
}
}
-*timeout = t;
}
void slirp_pollfds_fill(GArray *pollfds, uint32_t *timeout)
diff --git a/util/main-loop.c b/util/main-loop.c
index 992f9b0..fd23166 100644
--- a/util/main-loop.c
+++ b/util/main-loop.c
@@ -497,25 +497,16 @@ static int os_host_main_loop_wait(int64_t timeout)
void main_loop_wait(int nonblocking)
{
int ret;
-uint32_t timeout = UINT32_MAX;
int64_t timeout_ns;
+uint32_t timeout = nonblocking ? 0 : 1000 /* milliseconds */;
-if (nonblocking) {
-timeout = 0;
-}
/* poll any events */
g_array_set_size(gpollfds, 0); /* reset for new iteration */
/* XXX: separate device handlers from system ones */
slirp_pollfds_fill(gpollfds, );
-if (timeout == UINT32_MAX) {
-timeout_ns = -1;
-} else {
-timeout_ns = (uint64_t)timeout * (int64_t)(SCALE_MS);
-}
-
-timeout_ns = qemu_soonest_timeout(timeout_ns,
+timeout_ns = qemu_soonest_timeout((uint64_t)timeout * (int64_t)(SCALE_MS),
timerlistgroup_deadline_ns(
_loop_tlg));
--
2.7.4
[Qemu-devel] [Bug 1757363] Re: infinite loop due to improper deal with "eret" on mips32
What model/cpu is your router?
Which MIPS guest CPU are you using? Are you sure it matches the CPU of
your router?
Is your tplink firmware publicly available? (to reproduce your problem).
--
You received this bug notification because you are a member of qemu-
devel-ml, which is subscribed to QEMU.
https://bugs.launchpad.net/bugs/1757363
Title:
infinite loop due to improper deal with "eret" on mips32
Status in QEMU:
New
Bug description:
1.qemu 2.9.1 release on the official web build with tcg
2.cmd: qemu-system-mips -kernel kernelfile
3. host: ubuntu 16.04.1 with linux kernel 4.6.2 x86_64
guest: mips bigendian 32bit (tplink firmware)
detail:
static inline void exception_return(CPUMIPSState *env)
{
debug_pre_eret(env);
if (env->CP0_Status & (1 << CP0St_ERL)) {
set_pc(env, env->CP0_ErrorEPC);
env->CP0_Status &= ~(1 << CP0St_ERL);
} else {
set_pc(env, env->CP0_EPC);
env->CP0_Status &= ~(1 << CP0St_EXL);> ISSUE
}
compute_hflags(env);
debug_post_eret(env);
}
void helper_eret(CPUMIPSState *env)
{
exception_return(env);
env->lladdr = 1;
}
In the Issue Line, there is no check CP0_Status whether int is disabled
(should not enter int routine),
that result in the cpu can not jump out the int routine.
To manage notifications about this bug go to:
https://bugs.launchpad.net/qemu/+bug/1757363/+subscriptions
Re: [Qemu-devel] [Qemu-block] [PATCH 2/2] iotests: Add case for a corrupted inactive image
On Mon, Jun 04, 2018 at 04:14:37PM +0200, Max Reitz wrote:
> Signed-off-by: Max Reitz
Aborts without patch 1, passes with it, so a twofer:
Tested-by: Jeff Cody
Reviewed-by: Jeff Cody
> ---
> tests/qemu-iotests/060 | 30 ++
> tests/qemu-iotests/060.out | 14 ++
> 2 files changed, 44 insertions(+)
>
> diff --git a/tests/qemu-iotests/060 b/tests/qemu-iotests/060
> index 6c7407f499..7bdf609f3f 100755
> --- a/tests/qemu-iotests/060
> +++ b/tests/qemu-iotests/060
> @@ -440,6 +440,36 @@ echo "{'execute': 'qmp_capabilities'}
> -drive if=none,node-name=drive,file="$TEST_IMG",driver=qcow2 \
> | _filter_qmp | _filter_qemu_io
>
> +echo
> +echo "=== Testing incoming inactive corrupted image ==="
> +echo
> +
> +_make_test_img 64M
> +# Create an unaligned L1 entry, so qemu will signal a corruption when
> +# reading from the covered area
> +poke_file "$TEST_IMG" "$l1_offset" "\x00\x00\x00\x00\x2a\x2a\x2a\x2a"
> +
> +# Inactive images are effectively read-only images, so this should be a
> +# non-fatal corruption (which does not modify the image)
> +echo "{'execute': 'qmp_capabilities'}
> + {'execute': 'human-monitor-command',
> + 'arguments': {'command-line': 'qemu-io drive \"read 0 512\"'}}
> + {'execute': 'quit'}" \
> +| $QEMU -qmp stdio -nographic -nodefaults \
> +-blockdev "{'node-name': 'drive',
> +'driver': 'qcow2',
> +'file': {
> +'driver': 'file',
> +'filename': '$TEST_IMG'
> +}}" \
> +-incoming exec:'cat /dev/null' \
> +2>&1 \
> +| _filter_qmp | _filter_qemu_io
> +
> +echo
> +# Image should not have been marked corrupt
> +_img_info --format-specific | grep 'corrupt:'
> +
> # success, all done
> echo "*** done"
> rm -f $seq.full
> diff --git a/tests/qemu-iotests/060.out b/tests/qemu-iotests/060.out
> index 5f4264cff6..bff023d889 100644
> --- a/tests/qemu-iotests/060.out
> +++ b/tests/qemu-iotests/060.out
> @@ -420,4 +420,18 @@ write failed: Input/output error
> {"return": ""}
> {"return": {}}
> {"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event":
> "SHUTDOWN", "data": {"guest": false}}
> +
> +=== Testing incoming inactive corrupted image ===
> +
> +Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=67108864
> +QMP_VERSION
> +{"return": {}}
> +qcow2: Image is corrupt: L2 table offset 0x2a2a2a00 unaligned (L1 index: 0);
> further non-fatal corruption events will be suppressed
> +{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event":
> "BLOCK_IMAGE_CORRUPTED", "data": {"device": "", "msg": "L2 table offset
> 0x2a2a2a00 unaligned (L1 index: 0)", "node-name": "drive", "fatal": false}}
> +read failed: Input/output error
> +{"return": ""}
> +{"return": {}}
> +{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event":
> "SHUTDOWN", "data": {"guest": false}}
> +
> +corrupt: false
> *** done
> --
> 2.17.0
>
>
Re: [Qemu-devel] [PATCH 1/2] i386: define the AMD 'amd-ssbd' CPUID feature bit
On Fri, Jun 01, 2018 at 11:38:08AM -0400, Konrad Rzeszutek Wilk wrote:
> AMD future CPUs expose _two_ ways to utilize the Intel equivalant
> of the Speculative Store Bypass Disable. The first is via
> the virtualized VIRT_SPEC CTRL MSR (0xC001_011f) and the second
> is via the SPEC_CTRL MSR (0x48). The document titled:
> 124441_AMD64_SpeculativeStoreBypassDisable_Whitepaper_final.pdf
>
> gives priority of SPEC CTRL MSR over the VIRT SPEC CTRL MSR.
>
> A copy of this document is available at
> https://bugzilla.kernel.org/show_bug.cgi?id=199889
>
> Anyhow, this means that on future AMD CPUs there will be _two_ ways to
> deal with SSBD.
Does anybody know if there are AMD CPUs where virt-ssbd won't
work and would require amd-ssbd to mitigate vulnerabilities?
Also, do we have kernel arch/x86/kvm/cpuid.c patches, already?
I prefer to add new CPUID flag names only after the flag name is
already agreed upon on the kernel side.
>
> Signed-off-by: Konrad Rzeszutek Wilk
> ---
> target/i386/cpu.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/target/i386/cpu.c b/target/i386/cpu.c
> index 52d334a..f91990c 100644
> --- a/target/i386/cpu.c
> +++ b/target/i386/cpu.c
> @@ -490,7 +490,7 @@ static FeatureWordInfo feature_word_info[FEATURE_WORDS] =
> {
> "ibpb", NULL, NULL, NULL,
> NULL, NULL, NULL, NULL,
> NULL, NULL, NULL, NULL,
> -NULL, "virt-ssbd", NULL, NULL,
> +"amd-ssbd", "virt-ssbd", NULL, NULL,
> NULL, NULL, NULL, NULL,
> },
> .cpuid_eax = 0x8008,
> --
> 1.8.3.1
>
>
--
Eduardo
Re: [Qemu-devel] [Qemu-block] [PATCH 1/2] qcow2: Do not mark inactive images corrupt
On Mon, Jun 04, 2018 at 04:14:36PM +0200, Max Reitz wrote:
> When signaling a corruption on a read-only image, qcow2 already makes
> fatal events non-fatal (i.e., they will not result in the image being
> closed, and the image header's corrupt flag will not be set). This is
> necessary because we cannot set the corrupt flag on read-only images,
> and it is possible because further corruption of read-only images is
> impossible.
>
> Inactive images are effectively read-only, too, so we should do the same
> for them.
>
> (Otherwise, the assert(!(bs->open_flags & BDRV_O_INACTIVE)) in
> bdrv_co_pwritev() will fail, crashing qemu.)
>
> Cc: qemu-sta...@nongnu.org
> Signed-off-by: Max Reitz
> ---
> block/qcow2.c | 4 +++-
> 1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/block/qcow2.c b/block/qcow2.c
> index 59a38b9cd3..8b5f7386f7 100644
> --- a/block/qcow2.c
> +++ b/block/qcow2.c
> @@ -4402,7 +4402,9 @@ void qcow2_signal_corruption(BlockDriverState *bs, bool
> fatal, int64_t offset,
> char *message;
> va_list ap;
>
> -fatal = fatal && !bs->read_only;
> +if ((bs->open_flags & (BDRV_O_RDWR | BDRV_O_INACTIVE)) != BDRV_O_RDWR) {
Hmm, this is pretty much exactly what the bdrv_is_writable() helper function
does in block.c; too bad it's scope is limited to block.c. Maybe worth it
to make it a more widely available helper and use it here?
> +fatal = false;
> +}
>
> if (s->signaled_corruption &&
> (!fatal || (s->incompatible_features & QCOW2_INCOMPAT_CORRUPT)))
> --
> 2.17.0
>
>
Re: [Qemu-devel] [PATCH 09/33] linux-user: Set up infrastructure for table-izing syscalls
Le 01/06/2018 à 09:30, Richard Henderson a écrit : > Signed-off-by: Richard Henderson > --- > linux-user/syscall.c | 42 ++ > 1 file changed, 34 insertions(+), 8 deletions(-) > Reviewed-by: Laurent Vivier
Re: [Qemu-devel] [PATCH 08/33] linux-user: Make syscall number unsigned
Le 01/06/2018 à 09:30, Richard Henderson a écrit : > Signed-off-by: Richard Henderson > --- > linux-user/qemu.h| 2 +- > linux-user/syscall.c | 20 ++-- > 2 files changed, 11 insertions(+), 11 deletions(-) > Reviewed-by: Laurent Vivier
Re: [Qemu-devel] [PATCH 07/33] linux-user: Propagate goto fail to return
Le 01/06/2018 à 09:30, Richard Henderson a écrit :
> Signed-off-by: Richard Henderson
> ---
> linux-user/syscall.c | 62
> 1 file changed, 23 insertions(+), 39 deletions(-)
>
> @@ -9951,18 +9947,15 @@ static abi_long do_syscall1(void *cpu_env, int num,
> abi_long arg1,
> case TARGET_SYSLOG_ACTION_READ_CLEAR:/* Read/clear msgs */
> case TARGET_SYSLOG_ACTION_READ_ALL: /* Read last messages */
> {
> -ret = -TARGET_EINVAL;
> if (len < 0) {
> -goto fail;
> +return -TARGET_EINVAL;
> }
> -ret = 0;
> if (len == 0) {
> -return ret;
> +return 0;
I think you should do this change in '[PATCH 02/33] linux-user: Relax
single exit from "break"'
Reviewed-by: Laurent Vivier
Re: [Qemu-devel] [RFC PATCH] hw/registerfields: Deposit fields "in place"
On 06/04/2018 03:01 PM, Alistair Francis wrote:
> On Sat, Jun 2, 2018 at 3:26 PM, Philippe Mathieu-Daudé
> wrote:
>> On 06/02/2018 05:55 PM, Philippe Mathieu-Daudé wrote:
>>> These macros are always used to deposit a field in place.
>>> Update them to take the pointer argument.
>>>
>>> As these macros are constructed using compound statements,
>>> it is easy to not use the return value, and the compiler
>>> doesn't warn. Thus this change makes these macros safer.
>>
>> "and the compiler doesn't warn [if the return value is ignored]."
>>
>> Adding __attribute__ ((warn_unused_result)) to depositXX() doesn't help
>> since the retval is used inside the compound statement.
>
> Doesn't this then limit someone from writing an if statement around a
> value in a register?
It does indeed, but I'm not sure this would be a good code practice.
Also as you can see in this patch, there is no such use in the codebase.
I'd rather use 2 explicit steps, calling FIELD_EX64() first:
if (FIELD_EX64(storage, reg, field) == val1) {
FIELD_DP64(, reg, field, val2);
}
Maybe there is a way to write using __attribute__
((warn_unused_result)), eventually in 2 steps, 1 #define and 1 inlined func.
>
> Alistair
>
>>
>>>
>>> This also helps having a simpler/shorter code to review.
>>>
>>> Signed-off-by: Philippe Mathieu-Daudé
>>> ---
>>> hw/arm/smmuv3-internal.h| 2 +-
>>> include/hw/registerfields.h | 14 +-
I never noticed how git orderfile orders include...
>>> hw/arm/smmuv3.c | 28 +--
>>> hw/dma/xlnx-zdma.c | 6 ++--
>>> hw/sd/sd.c | 4 +--
>>> hw/sd/sdhci.c | 56 ++---
>>> 6 files changed, 53 insertions(+), 57 deletions(-)
>>>
>>> diff --git a/hw/arm/smmuv3-internal.h b/hw/arm/smmuv3-internal.h
>>> index a9d714b56e..2f020e2e90 100644
>>> --- a/hw/arm/smmuv3-internal.h
>>> +++ b/hw/arm/smmuv3-internal.h
>>> @@ -203,7 +203,7 @@ static inline bool smmuv3_eventq_enabled(SMMUv3State *s)
>>>
>>> static inline void smmu_write_cmdq_err(SMMUv3State *s, uint32_t err_type)
>>> {
>>> -s->cmdq.cons = FIELD_DP32(s->cmdq.cons, CMDQ_CONS, ERR, err_type);
>>> +FIELD_DP32(>cmdq.cons, CMDQ_CONS, ERR, err_type);
>>> }
>>>
>>> /* Commands */
>>> diff --git a/include/hw/registerfields.h b/include/hw/registerfields.h
>>> index 2659a58737..14a12f6a48 100644
>>> --- a/include/hw/registerfields.h
>>> +++ b/include/hw/registerfields.h
>>> @@ -45,7 +45,7 @@
>>> #define ARRAY_FIELD_EX32(regs, reg, field)\
>>> FIELD_EX32((regs)[R_ ## reg], reg, field)
>>>
>>> -/* Deposit a register field.
>>> +/* Deposit a register field (in place).
>>> * Assigning values larger then the target field will result in
>>> * compilation warnings.
>>> */
>>> @@ -54,20 +54,20 @@
>>> unsigned int v:R_ ## reg ## _ ## field ## _LENGTH;\
>>> } v = { .v = val }; \
>>> uint32_t d; \
>>> -d = deposit32((storage), R_ ## reg ## _ ## field ## _SHIFT, \
>>> - R_ ## reg ## _ ## field ## _LENGTH, v.v); \
>>> -d; })
>>> +d = deposit32(*(storage), R_ ## reg ## _ ## field ## _SHIFT, \
>>> + R_ ## reg ## _ ## field ## _LENGTH, v.v);
>>> \
>>
>> I forgot to remove an extra space here.
>>
>>> +*(storage) = d; })
>>> #define FIELD_DP64(storage, reg, field, val) ({ \
>>> struct { \
>>> unsigned int v:R_ ## reg ## _ ## field ## _LENGTH;\
>>> } v = { .v = val }; \
>>> uint64_t d; \
>>> -d = deposit64((storage), R_ ## reg ## _ ## field ## _SHIFT, \
>>> +d = deposit64(*(storage), R_ ## reg ## _ ## field ## _SHIFT, \
>>>R_ ## reg ## _ ## field ## _LENGTH, v.v); \
>>> -d; })
>>> +*(storage) = d; })
>>>
>>> /* Deposit a field to array of registers. */
>>> #define ARRAY_FIELD_DP32(regs, reg, field, val) \
>>> -(regs)[R_ ## reg] = FIELD_DP32((regs)[R_ ## reg], reg, field, val);
>>> +FIELD_DP32(&(regs)[R_ ## reg], reg, field, val);
>>>
>>> #endif
>>> diff --git a/hw/arm/smmuv3.c b/hw/arm/smmuv3.c
>>> index 42dc521c13..6406b69d68 100644
>>> --- a/hw/arm/smmuv3.c
>>> +++ b/hw/arm/smmuv3.c
>>> @@ -238,25 +238,25 @@ static void smmuv3_init_regs(SMMUv3State *s)
>>> * IDR0: stage1 only, AArch64 only, coherent access, 16b ASID,
>>> * multi-level stream table
>>> */
>>> -s->idr[0] = FIELD_DP32(s->idr[0], IDR0, S1P, 1); /* stage 1 supported
>>> */
>>> -s->idr[0] = FIELD_DP32(s->idr[0], IDR0, TTF, 2); /* AArch64 PTW only */
>>>
Re: [Qemu-devel] [PATCH 06/33] linux-user: Split out goto unimplemented to do_unimplemented
Le 01/06/2018 à 09:30, Richard Henderson a écrit : > Signed-off-by: Richard Henderson > --- > linux-user/syscall.c | 82 +++- > 1 file changed, 43 insertions(+), 39 deletions(-) > Reviewed-by: Laurent Vivier
Re: [Qemu-devel] [PATCH 05/33] linux-user: Propagate goto unimplemented_nowarn to return
Le 01/06/2018 à 09:30, Richard Henderson a écrit : > Signed-off-by: Richard Henderson > --- > linux-user/syscall.c | 11 --- > 1 file changed, 4 insertions(+), 7 deletions(-) > Reviewed-by: Laurent Vivier
Re: [Qemu-devel] [PATCH 04/33] linux-user: Propagate goto efault to return
Le 01/06/2018 à 09:30, Richard Henderson a écrit : > Signed-off-by: Richard Henderson > --- > linux-user/syscall.c | 311 +-- > 1 file changed, 154 insertions(+), 157 deletions(-) > Reviewed-by: Laurent Vivier
Re: [Qemu-devel] [PATCH 03/33] linux-user: Propagate goto ebadf to return
Le 01/06/2018 à 09:30, Richard Henderson a écrit : > Signed-off-by: Richard Henderson > --- > linux-user/syscall.c | 187 +-- > 1 file changed, 92 insertions(+), 95 deletions(-) > Reviewed-by: Laurent Vivier
Re: [Qemu-devel] [PATCH 02/33] linux-user: Relax single exit from "break"
Le 01/06/2018 à 09:30, Richard Henderson a écrit : > Transform outermost "break" to "return ret". If the immediately > preceeding statement was an assignment to ret, return the value > directly. > > Signed-off-by: Richard Henderson > --- > linux-user/syscall.c | 969 +-- > 1 file changed, 390 insertions(+), 579 deletions(-) Reviewed-by: Laurent Vivier
Re: [Qemu-devel] [PATCH] CODING_STYLE: Define our preferred form for multiline comments
On 06/04/2018 01:21 PM, Peter Maydell wrote: > The codebase has a bit of a mix of > /* multiline comments > * like this > */ > and > /* multiline comments like this > in the GNU Coding Standards style */ > > State a preference for the former. > > Signed-off-by: Peter Maydell > --- > I admit that to some extent I'm imposing my aesthetic > preferences here; pretty sure we have a lot more style > 1 comments than style 2, though. The later is easier to indent with unintelligent editors. Any one is fine as long as it has a guideline in coding style. > --- > CODING_STYLE | 13 + > 1 file changed, 13 insertions(+) > > diff --git a/CODING_STYLE b/CODING_STYLE > index 12ba58ee293..fb1d1f1cd62 100644 > --- a/CODING_STYLE > +++ b/CODING_STYLE > @@ -124,6 +124,19 @@ We use traditional C-style /* */ comments and avoid // > comments. > Rationale: The // form is valid in C99, so this is purely a matter of > consistency of style. The checkpatch script will warn you about this. > > +Multiline comments blocks should have a row of stars on the left > +and the terminating */ on its own line: > +/* like > + * this > + */ > +Putting the initial /* on its own line is accepted, but not required. > +(Some of the existing comments in the codebase use the GNU Coding > +Standards form which does not have stars on the left; avoid this > +when writing new comments.) > + > +Rationale: Consistency, and ease of visually picking out a multiline > +comment from the surrounding code. Reviewed-by: Philippe Mathieu-Daudé > + > 8. trace-events style > > 8.1 0x prefix >
Re: [Qemu-devel] [Qemu-block] [PATCH 0/2] qcow2: Do not mark inactive images corrupt
On 06/04/2018 10:14 AM, Max Reitz wrote: > The non-public logs in > https://bugzilla.redhat.com/show_bug.cgi?id=1583346 (sorry...) reveal > this problem: > > $ (Create a qcow2 file "foo.qcow2" with a corrupted first L1 entry) > $ echo 'qemu-io none0 "read 0 512"' \ > | x86_64-softmmu/qemu-system-x86_64 -drive if=none,file=foo.qcow2 \ > -monitor stdio \ > -incoming exec:'cat /dev/null' > QEMU 2.12.50 monitor - type 'help' for more information > (qemu) qemu-io none0 "read 0 512" > qcow2: Marking image as corrupt: L2 table offset 0x44200 unaligned (L1 index: > 0); further corruption events will be suppressed > qemu-system-x86_64: block/io.c:1691: bdrv_co_pwritev: Assertion > `!(bs->open_flags & BDRV_O_INACTIVE)' failed. > [1]18444 done echo 'qemu-io none0 "read 0 512"' | >18445 abort (core dumped) x86_64-softmmu/qemu-system-x86_64 -drive > if=none,file=foo.qcow2 -monitor stdi > > Oops. > > > The first patch in this series fixes this by treating inactive images > like read-only images in this regard (which most importantly means not > trying to set the corrupt flag on them), the second one adds an iotest > case. > > > Max Reitz (2): > qcow2: Do not mark inactive images corrupt > iotests: Add case for a corrupted inactive image > > block/qcow2.c | 4 +++- > tests/qemu-iotests/060 | 30 ++ > tests/qemu-iotests/060.out | 14 ++ > 3 files changed, 47 insertions(+), 1 deletion(-) > Makes sense to me, provided it's safe to check via BDRV_O_RDWR instead of bs->read_only. (I assume it is.) Reviewed-by: John Snow
Re: [Qemu-devel] [PATCH RFC] hw/pc: set q35 as the default x86 machine
On 06/04/2018 07:56 PM, Daniel P. Berrangé wrote: On Mon, Jun 04, 2018 at 07:48:51PM +0300, Michael S. Tsirkin wrote: On Mon, Jun 04, 2018 at 02:01:29PM +0100, Daniel P. Berrangé wrote: On Mon, Jun 04, 2018 at 09:54:15AM -0300, Eduardo Habkost wrote: On Mon, Jun 04, 2018 at 04:38:22AM +0300, Michael S. Tsirkin wrote: On Sun, Jun 03, 2018 at 12:27:49PM +0300, Marcel Apfelbaum wrote: Moving to QEMU 3.0 seems like a good opportunity for such a change. I440FX is really old and does not support modern features like IOMMU. Q35's SATA emulation is faster than pc's IDE, native PCI express hotplug is cleaner than ACPI based one and so on... Also the libvirt guys added very good support for the Q35 machine (thanks!). Management software should always specify the machine type and for the current setups, adding '-machine pc' to the command line is not such a big deal. In time the pc machine will fade out and we will probably stop adding new versions at some point. Signed-off-by: Marcel Apfelbaum For command line users, I think changing the default isn't nice. Yes it's easy to add -machine pc but there's no documentation that tells you to do so. Add to that shortcuts like -cdrom stop working, hotplug needs extra bridges to work, and one can see that while management tool users benefit from q35, command line users will suffer. Can't we add a tag for management without changing the command line default? How about "management-default"? "recommended"? "latest"? We could add new aliases if they are useful for management software, but we would need a well-defined use case and set of requirements+expectations for the new alias. I'm not convinced by the idea of adding a distinct default "for mgmt". All the problems described wrt 'q35' vs 'pc' apply equally to management apps as they do to humans. It just happens that one common mgmt layer (libvirt) knows how to handle some of the complexity of q35. Other mgmt apps though are just as likely to be hurt by the change as humans are. So effectively the proposed "for mgmt" is actually "for libvirt >= some version", which feels like a layering violation to me. Is libvirt happy to just hard-code q35 for now then? I'm pretty wary of doing that, as I feel 'pc' has broader OS compatibility than 'q35', so we'd be likely to cause breakage for users. IMHO, defaults are something better expressed in libosinfo, so if there's guests where we think q35 is a better choice, we can record it there and leave everything else on pc to avoid risk of breakage. The only info we need to pass properly to management systems is: "Use q35 unless your guests are really old". I agree the exiting systems should not be touched. Thanks, Marcel Regards, Daniel
Re: [Qemu-devel] [PATCH RFC] hw/pc: set q35 as the default x86 machine
On Mon, Jun 04, 2018 at 08:17:23PM +0300, Michael S. Tsirkin wrote: > On Mon, Jun 04, 2018 at 10:26:24AM -0300, Eduardo Habkost wrote: > > On Mon, Jun 04, 2018 at 02:01:29PM +0100, Daniel P. Berrangé wrote: > > > On Mon, Jun 04, 2018 at 09:54:15AM -0300, Eduardo Habkost wrote: > > > > On Mon, Jun 04, 2018 at 04:38:22AM +0300, Michael S. Tsirkin wrote: > > > > > On Sun, Jun 03, 2018 at 12:27:49PM +0300, Marcel Apfelbaum wrote: > > > > > > Moving to QEMU 3.0 seems like a good opportunity for such a change. > > > > > > > > > > > > I440FX is really old and does not support modern features like > > > > > > IOMMU. > > > > > > Q35's SATA emulation is faster than pc's IDE, native PCI express > > > > > > hotplug > > > > > > is cleaner than ACPI based one and so on... > > > > > > > > > > > > Also the libvirt guys added very good support for the Q35 machine > > > > > > (thanks!). > > > > > > > > > > > > Management software should always specify the machine type and for > > > > > > the > > > > > > current setups, adding '-machine pc' to the command line is not > > > > > > such a > > > > > > big deal. > > > > > > > > > > > > In time the pc machine will fade out and we will probably stop > > > > > > adding > > > > > > new versions at some point. > > > > > > > > > > > > Signed-off-by: Marcel Apfelbaum > > > > > > > > > > For command line users, I think changing the default isn't nice. > > > > > > > > > > Yes it's easy to add -machine pc but there's no documentation > > > > > that tells you to do so. Add to that shortcuts like -cdrom > > > > > stop working, hotplug needs extra bridges to work, and one > > > > > can see that while management tool users benefit from q35, > > > > > command line users will suffer. > > > > > > > > > > Can't we add a tag for management without changing the command line > > > > > default? How about "management-default"? "recommended"? "latest"? > > > > > > > > We could add new aliases if they are useful for management > > > > software, but we would need a well-defined use case and set of > > > > requirements+expectations for the new alias. > > > > > > I'm not convinced by the idea of adding a distinct default "for mgmt". All > > > the problems described wrt 'q35' vs 'pc' apply equally to management apps > > > as they do to humans. It just happens that one common mgmt layer (libvirt) > > > knows how to handle some of the complexity of q35. Other mgmt apps though > > > are just as likely to be hurt by the change as humans are. So effectively > > > the proposed "for mgmt" is actually "for libvirt >= some version", which > > > feels like a layering violation to me. > > > > This means the new alias would be used only if requested > > explicitly by management software (not used automatically by > > libvirt). > > > > Taking that into account, I still don't see what exactly would be > > the use case here, and what exactly users can/can't expect when > > using the new alias. > > Let's see what we have now first: > > 1. We have a requirement for the user to save the machine type on install > and maintain it with the image (a separate thread discusses saving that > as part of a qcow2 image). > > 2. If you use an alias instead you are supposed to resolve it > and save the resolved value. If you save the alias instead, > you can't do cross-version live migration. > > 3. If you don't specify anything you get a machine tagged default. You are > supposed to find it and save the value found. If you don't and just > keep using the default, you can't do cross-version live migration. > > --- > > So now we would like to relax 3 to say > "If you don't and just keep using the default, some images might not > boot". > > unfortunately we probably can't change 3 like this. > So what I propose instead is simply: > > 4. If you find a machine type value tagged "qmp-default" you must save >the value found. If you don't and just keep using the qmp-default each >time, then existing guest images won't boot. >This relaxed compatibility requirement allows for advanced features >as compared to default. What problems would this system solve? Who would prefer to use the "qmp-default" machine-type instead of the "pc" or "q35" aliases? -- Eduardo
Re: [Qemu-devel] [PATCH RFC] hw/pc: set q35 as the default x86 machine
Hi Michael, On 06/04/2018 04:38 AM, Michael S. Tsirkin wrote: On Sun, Jun 03, 2018 at 12:27:49PM +0300, Marcel Apfelbaum wrote: Moving to QEMU 3.0 seems like a good opportunity for such a change. I440FX is really old and does not support modern features like IOMMU. Q35's SATA emulation is faster than pc's IDE, native PCI express hotplug is cleaner than ACPI based one and so on... Also the libvirt guys added very good support for the Q35 machine (thanks!). Management software should always specify the machine type and for the current setups, adding '-machine pc' to the command line is not such a big deal. In time the pc machine will fade out and we will probably stop adding new versions at some point. Signed-off-by: Marcel Apfelbaum For command line users, I think changing the default isn't nice. Yes it's easy to add -machine pc but there's no documentation that tells you to do so. We can add something do the help. Add to that shortcuts like -cdrom stop working, Maybe is fixable. hotplug needs extra bridges to work, Adding a pci express root port in case hotplug is desired should not be too hard. and one can see that while management tool users benefit from q35, command line users will suffer. Can't we add a tag for management without changing the command line default? How about "management-default"? "recommended"? "latest"? This will help maybe, but was not the point. We have two x86 machine types, meaning some features will be developed/tested for pc, while others for q35. At some point we will loose track of what is working for each machine. The PC machine command line is simpler and it supports older guest OSes, so we should keep it, of course; but I am not sure we should add more features to it. I see marking Q35 as the default machine a first step. Thanks, Marcel
Re: [Qemu-devel] [PATCH 5/8] hw/sd/ssi-sd: Force cards connected in SPI mode to use Spec v1.10
On Sat, Jun 2, 2018 at 5:08 PM, Philippe Mathieu-Daudé wrote:
> Signed-off-by: Philippe Mathieu-Daudé
Can you add a justification for this in the commit message?
Alistair
> ---
> hw/sd/ssi-sd.c | 2 ++
> 1 file changed, 2 insertions(+)
>
> diff --git a/hw/sd/ssi-sd.c b/hw/sd/ssi-sd.c
> index ae04b6641b..c62fdc871c 100644
> --- a/hw/sd/ssi-sd.c
> +++ b/hw/sd/ssi-sd.c
> @@ -253,6 +253,8 @@ static void ssi_sd_realize(SSISlave *d, Error **errp)
> /* FIXME use a qdev drive property instead of drive_get_next() */
> dinfo = drive_get_next(IF_SD);
> carddev = qdev_create(>sdbus.qbus, TYPE_SD_CARD);
> +object_property_set_uint(OBJECT(carddev),
> + SD_PHY_SPECv1_10_VERS, "spec_version", );
> if (dinfo) {
> qdev_prop_set_drive(carddev, "drive", blk_by_legacy_dinfo(dinfo),
> );
> }
> --
> 2.17.1
>
>
Re: [Qemu-devel] [PATCH 4/8] sdcard: Set Spec v2.00 as default
On Sat, Jun 2, 2018 at 5:08 PM, Philippe Mathieu-Daudé wrote:
> Signed-off-by: Philippe Mathieu-Daudé
Reviewed-by: Alistair Francis
Alistair
> ---
> include/hw/sd/sd.h | 1 +
> hw/sd/sd.c | 13 +
> 2 files changed, 10 insertions(+), 4 deletions(-)
>
> diff --git a/include/hw/sd/sd.h b/include/hw/sd/sd.h
> index 49f22b0b89..7c6ad3c8f1 100644
> --- a/include/hw/sd/sd.h
> +++ b/include/hw/sd/sd.h
> @@ -56,6 +56,7 @@
>
> enum SDPhySpecificationVersion {
> SD_PHY_SPECv1_10_VERS = 1,
> +SD_PHY_SPECv2_00_VERS = 2,
> };
>
> typedef enum {
> diff --git a/hw/sd/sd.c b/hw/sd/sd.c
> index 5ddd24..81f178b36e 100644
> --- a/hw/sd/sd.c
> +++ b/hw/sd/sd.c
> @@ -311,8 +311,12 @@ static void sd_ocr_powerup(void *opaque)
>
> static void sd_set_scr(SDState *sd)
> {
> -sd->scr[0] = (0 << 4) /* SCR structure version 1.0 */
> - | 1; /* Spec Version 1.10 */
> +sd->scr[0] = 0 << 4;/* SCR structure version 1.0 */
> +if (sd->spec_version == SD_PHY_SPECv1_10_VERS) {
> +sd->scr[0] |= 1;/* Spec Version 1.10 */
> +} else {
> +sd->scr[0] |= 2;/* Spec Version 2.00 */
> +}
> sd->scr[1] = (2 << 4) /* SDSC Card (Security Version 1.01) */
> | 0b0101; /* 1-bit or 4-bit width bus modes */
> sd->scr[2] = 0x00; /* Extended Security is not supported. */
> @@ -2060,7 +2064,8 @@ static void sd_realize(DeviceState *dev, Error **errp)
> sd->proto_name = sd->spi ? "SPI" : "SD";
>
> switch (sd->spec_version) {
> -case SD_PHY_SPECv1_10_VERS:
> +case SD_PHY_SPECv1_10_VERS
> + ... SD_PHY_SPECv2_00_VERS:
> break;
> default:
> error_setg(errp, "Invalid SD card Spec version: %u",
> sd->spec_version);
> @@ -2084,7 +2089,7 @@ static void sd_realize(DeviceState *dev, Error **errp)
>
> static Property sd_properties[] = {
> DEFINE_PROP_UINT8("spec_version", SDState,
> - spec_version, SD_PHY_SPECv1_10_VERS),
> + spec_version, SD_PHY_SPECv2_00_VERS),
> DEFINE_PROP_DRIVE("drive", SDState, blk),
> /* We do not model the chip select pin, so allow the board to select
> * whether card should be in SSI or MMC/SD mode. It is also up to the
> --
> 2.17.1
>
>
Re: [Qemu-devel] [PATCH 3/8] sdcard: Add a 'spec_version' property
On Sat, Jun 2, 2018 at 5:08 PM, Philippe Mathieu-Daudé wrote:
> Signed-off-by: Philippe Mathieu-Daudé
Reviewed-by: Alistair Francis
Alistair
> ---
> include/hw/sd/sd.h | 4
> hw/sd/sd.c | 11 +++
> 2 files changed, 15 insertions(+)
>
> diff --git a/include/hw/sd/sd.h b/include/hw/sd/sd.h
> index 9bdb3c9285..49f22b0b89 100644
> --- a/include/hw/sd/sd.h
> +++ b/include/hw/sd/sd.h
> @@ -54,6 +54,10 @@
> #define APP_CMD(1 << 5)
> #define AKE_SEQ_ERROR (1 << 3)
>
> +enum SDPhySpecificationVersion {
> +SD_PHY_SPECv1_10_VERS = 1,
> +};
> +
> typedef enum {
> SD_VOLTAGE_0_4V = 400, /* currently not supported */
> SD_VOLTAGE_1_8V = 1800,
> diff --git a/hw/sd/sd.c b/hw/sd/sd.c
> index 80e70dd93e..5ddd24 100644
> --- a/hw/sd/sd.c
> +++ b/hw/sd/sd.c
> @@ -91,6 +91,7 @@ struct SDState {
> uint8_t sd_status[64];
>
> /* Configurable properties */
> +uint8_t spec_version;
> BlockBackend *blk;
> bool spi;
>
> @@ -2058,6 +2059,14 @@ static void sd_realize(DeviceState *dev, Error **errp)
>
> sd->proto_name = sd->spi ? "SPI" : "SD";
>
> +switch (sd->spec_version) {
> +case SD_PHY_SPECv1_10_VERS:
> +break;
> +default:
> +error_setg(errp, "Invalid SD card Spec version: %u",
> sd->spec_version);
> +return;
> +}
> +
> if (sd->blk && blk_is_read_only(sd->blk)) {
> error_setg(errp, "Cannot use read-only drive as SD card");
> return;
> @@ -2074,6 +2083,8 @@ static void sd_realize(DeviceState *dev, Error **errp)
> }
>
> static Property sd_properties[] = {
> +DEFINE_PROP_UINT8("spec_version", SDState,
> + spec_version, SD_PHY_SPECv1_10_VERS),
> DEFINE_PROP_DRIVE("drive", SDState, blk),
> /* We do not model the chip select pin, so allow the board to select
> * whether card should be in SSI or MMC/SD mode. It is also up to the
> --
> 2.17.1
>
>
Re: [Qemu-devel] [PATCH 2/8] sdcard: Allow commands valid in SPI mode
On Sat, Jun 2, 2018 at 5:08 PM, Philippe Mathieu-Daudé wrote:
> From the "Physical Layer Simplified Specification Version 1.10"
> Chapter 7.3 "SPI Mode Transaction Packets"
> Table 57: "Commands and arguments"
>
> Signed-off-by: Philippe Mathieu-Daudé
Acked-by: Alistair Francis
Alistair
> ---
> hw/sd/sd.c | 14 --
> 1 file changed, 14 deletions(-)
>
> diff --git a/hw/sd/sd.c b/hw/sd/sd.c
> index e1218d1fb6..80e70dd93e 100644
> --- a/hw/sd/sd.c
> +++ b/hw/sd/sd.c
> @@ -960,8 +960,6 @@ static sd_rsp_type_t sd_normal_command(SDState *sd,
> SDRequest req)
> return sd_illegal;
>
> case 6:/* CMD6: SWITCH_FUNCTION */
> -if (sd->spi)
> -goto bad_cmd;
> switch (sd->mode) {
> case sd_data_transfer_mode:
> sd_function_switch(sd, req.arg);
> @@ -1190,9 +1188,6 @@ static sd_rsp_type_t sd_normal_command(SDState *sd,
> SDRequest req)
>
> /* Block write commands (Class 4) */
> case 24: /* CMD24: WRITE_SINGLE_BLOCK */
> -if (sd->spi) {
> -goto unimplemented_spi_cmd;
> -}
> switch (sd->state) {
> case sd_transfer_state:
> /* Writing in SPI mode not implemented. */
> @@ -1217,9 +1212,6 @@ static sd_rsp_type_t sd_normal_command(SDState *sd,
> SDRequest req)
> break;
>
> case 25: /* CMD25: WRITE_MULTIPLE_BLOCK */
> -if (sd->spi) {
> -goto unimplemented_spi_cmd;
> -}
> switch (sd->state) {
> case sd_transfer_state:
> /* Writing in SPI mode not implemented. */
> @@ -1259,9 +1251,6 @@ static sd_rsp_type_t sd_normal_command(SDState *sd,
> SDRequest req)
> break;
>
> case 27: /* CMD27: PROGRAM_CSD */
> -if (sd->spi) {
> -goto unimplemented_spi_cmd;
> -}
> switch (sd->state) {
> case sd_transfer_state:
> sd->state = sd_receivingdata_state;
> @@ -1371,9 +1360,6 @@ static sd_rsp_type_t sd_normal_command(SDState *sd,
> SDRequest req)
>
> /* Lock card commands (Class 7) */
> case 42: /* CMD42: LOCK_UNLOCK */
> -if (sd->spi) {
> -goto unimplemented_spi_cmd;
> -}
> switch (sd->state) {
> case sd_transfer_state:
> sd->state = sd_receivingdata_state;
> --
> 2.17.1
>
>
Re: [Qemu-devel] [PATCH 0/8] sdcard: cleanup the SD_SPEC version
On Sat, Jun 2, 2018 at 5:08 PM, Philippe Mathieu-Daudé wrote: > Hi, > > This series adds a 'spec_version' property to the SD Card device, > to allow to limit some commands to specific spec version range > (some firmwares use this feature to detect which spec version the > card implements). > > This restore the SSI/SD support of the Stellaris LM3S6965EVB board, > which allow to stress the SD Card code with a nice integration test > (waiting for another series to get merged to add the Avocado test): > > $ qemu-system-arm -M lm3s6965evb -serial stdio \ > -kernel sd_card.bin -sd sdcard.img > > SD Card Example Program > Type 'help' for help. > > /> ls > Open > listing > > A 2012/04/25 17:4412 README.TXT > >1 File(s),12 bytes total >0 Dir(s), 61182K bytes free > > /> cat README.TXT > Hello World > > See: > http://lists.nongnu.org/archive/html/qemu-devel/2012-04/msg03790.html > http://lists.nongnu.org/archive/html/qemu-devel/2018-06/msg00353.html Do you have a branch with all of your SD work available? Alistair > > Regards, > > Phil. > > Philippe Mathieu-Daudé (8): > sdcard: Update the Configuration Register (SCR) to Spec Version 1.10 > sdcard: Allow commands valid in SPI mode > sdcard: Add a 'spec_version' property > sdcard: Set Spec v2.00 as default > hw/sd/ssi-sd: Force cards connected in SPI mode to use Spec v1.10 > sdcard: Disable SEND_IF_COND (CMD8) for Spec v1 > sdcard: Reflect when the Spec v3 is supported in the Config Register (SCR) > sdcard: Disable CMD19/CMD23 for Spec v2 > > include/hw/sd/sd.h | 6 ++ > hw/sd/sd.c | 47 +- > hw/sd/ssi-sd.c | 2 ++ > 3 files changed, 38 insertions(+), 17 deletions(-) > > -- > 2.17.1 > >
Re: [Qemu-devel] [PATCH RFC] hw/pc: set q35 as the default x86 machine
On 06/04/2018 08:35 AM, Paolo Bonzini wrote: > On 04/06/2018 14:24, Igor Mammedov wrote: >>> Yes it's easy to add -machine pc but there's no documentation >>> that tells you to do so. Add to that shortcuts like -cdrom >>> stop working, hotplug needs extra bridges to work, and one >>> can see that while management tool users benefit from q35, >>> command line users will suffer. >> Maybe we should mark 'pc' default as deprecated first, >> like we do with CLI options that we wish to drop in future? >> >> That way we 'create' documentation, so users would be aware >> of the change and have time to fix their CLI if they prefer >> 'pc' machine. > > Michael has listed reasons why 'pc' cannot be deprecated, since some of > them are not even fixable. Honestly, 'pc' works well for 99% of the use > cases---just like you probably don't care whether your laptop has a PCI > or PCIe chipset. > > Paolo > > I understood this comment to mean deprecating a *default* machine type. So if you do `-M pc` you're still OK, but if you simply omit a machine type and hope for a specific one you're out of luck. ... We could just deprecate having any default and then make QEMU whine at you if you don't specify one, like we do for guessing format types on drive images -- it'll do it, but if it guesses raw it whines at you a little.
Re: [Qemu-devel] [RFC PATCH] hw/registerfields: Deposit fields "in place"
On Sat, Jun 2, 2018 at 3:26 PM, Philippe Mathieu-Daudé wrote:
> On 06/02/2018 05:55 PM, Philippe Mathieu-Daudé wrote:
>> These macros are always used to deposit a field in place.
>> Update them to take the pointer argument.
>>
>> As these macros are constructed using compound statements,
>> it is easy to not use the return value, and the compiler
>> doesn't warn. Thus this change makes these macros safer.
>
> "and the compiler doesn't warn [if the return value is ignored]."
>
> Adding __attribute__ ((warn_unused_result)) to depositXX() doesn't help
> since the retval is used inside the compound statement.
Doesn't this then limit someone from writing an if statement around a
value in a register?
Alistair
>
>>
>> This also helps having a simpler/shorter code to review.
>>
>> Signed-off-by: Philippe Mathieu-Daudé
>> ---
>> hw/arm/smmuv3-internal.h| 2 +-
>> include/hw/registerfields.h | 14 +-
>> hw/arm/smmuv3.c | 28 +--
>> hw/dma/xlnx-zdma.c | 6 ++--
>> hw/sd/sd.c | 4 +--
>> hw/sd/sdhci.c | 56 ++---
>> 6 files changed, 53 insertions(+), 57 deletions(-)
>>
>> diff --git a/hw/arm/smmuv3-internal.h b/hw/arm/smmuv3-internal.h
>> index a9d714b56e..2f020e2e90 100644
>> --- a/hw/arm/smmuv3-internal.h
>> +++ b/hw/arm/smmuv3-internal.h
>> @@ -203,7 +203,7 @@ static inline bool smmuv3_eventq_enabled(SMMUv3State *s)
>>
>> static inline void smmu_write_cmdq_err(SMMUv3State *s, uint32_t err_type)
>> {
>> -s->cmdq.cons = FIELD_DP32(s->cmdq.cons, CMDQ_CONS, ERR, err_type);
>> +FIELD_DP32(>cmdq.cons, CMDQ_CONS, ERR, err_type);
>> }
>>
>> /* Commands */
>> diff --git a/include/hw/registerfields.h b/include/hw/registerfields.h
>> index 2659a58737..14a12f6a48 100644
>> --- a/include/hw/registerfields.h
>> +++ b/include/hw/registerfields.h
>> @@ -45,7 +45,7 @@
>> #define ARRAY_FIELD_EX32(regs, reg, field)\
>> FIELD_EX32((regs)[R_ ## reg], reg, field)
>>
>> -/* Deposit a register field.
>> +/* Deposit a register field (in place).
>> * Assigning values larger then the target field will result in
>> * compilation warnings.
>> */
>> @@ -54,20 +54,20 @@
>> unsigned int v:R_ ## reg ## _ ## field ## _LENGTH;\
>> } v = { .v = val }; \
>> uint32_t d; \
>> -d = deposit32((storage), R_ ## reg ## _ ## field ## _SHIFT, \
>> - R_ ## reg ## _ ## field ## _LENGTH, v.v); \
>> -d; })
>> +d = deposit32(*(storage), R_ ## reg ## _ ## field ## _SHIFT, \
>> + R_ ## reg ## _ ## field ## _LENGTH, v.v);\
>
> I forgot to remove an extra space here.
>
>> +*(storage) = d; })
>> #define FIELD_DP64(storage, reg, field, val) ({ \
>> struct { \
>> unsigned int v:R_ ## reg ## _ ## field ## _LENGTH;\
>> } v = { .v = val }; \
>> uint64_t d; \
>> -d = deposit64((storage), R_ ## reg ## _ ## field ## _SHIFT, \
>> +d = deposit64(*(storage), R_ ## reg ## _ ## field ## _SHIFT, \
>>R_ ## reg ## _ ## field ## _LENGTH, v.v); \
>> -d; })
>> +*(storage) = d; })
>>
>> /* Deposit a field to array of registers. */
>> #define ARRAY_FIELD_DP32(regs, reg, field, val) \
>> -(regs)[R_ ## reg] = FIELD_DP32((regs)[R_ ## reg], reg, field, val);
>> +FIELD_DP32(&(regs)[R_ ## reg], reg, field, val);
>>
>> #endif
>> diff --git a/hw/arm/smmuv3.c b/hw/arm/smmuv3.c
>> index 42dc521c13..6406b69d68 100644
>> --- a/hw/arm/smmuv3.c
>> +++ b/hw/arm/smmuv3.c
>> @@ -238,25 +238,25 @@ static void smmuv3_init_regs(SMMUv3State *s)
>> * IDR0: stage1 only, AArch64 only, coherent access, 16b ASID,
>> * multi-level stream table
>> */
>> -s->idr[0] = FIELD_DP32(s->idr[0], IDR0, S1P, 1); /* stage 1 supported */
>> -s->idr[0] = FIELD_DP32(s->idr[0], IDR0, TTF, 2); /* AArch64 PTW only */
>> -s->idr[0] = FIELD_DP32(s->idr[0], IDR0, COHACC, 1); /* IO coherent */
>> -s->idr[0] = FIELD_DP32(s->idr[0], IDR0, ASID16, 1); /* 16-bit ASID */
>> -s->idr[0] = FIELD_DP32(s->idr[0], IDR0, TTENDIAN, 2); /* little endian
>> */
>> -s->idr[0] = FIELD_DP32(s->idr[0], IDR0, STALL_MODEL, 1); /* No stall */
>> +FIELD_DP32(>idr[0], IDR0, S1P, 1); /* stage 1 supported */
>> +FIELD_DP32(>idr[0], IDR0, TTF, 2); /* AArch64 PTW only */
>> +FIELD_DP32(>idr[0], IDR0, COHACC, 1); /* IO coherent */
>> +FIELD_DP32(>idr[0], IDR0, ASID16, 1); /* 16-bit ASID */
>> +FIELD_DP32(>idr[0], IDR0, TTENDIAN, 2); /* little endian */
Re: [Qemu-devel] [PATCH] CODING_STYLE: Define our preferred form for multiline comments
On 06/04/2018 12:21 PM, Peter Maydell wrote: > The codebase has a bit of a mix of > /* multiline comments > * like this > */ > and > /* multiline comments like this > in the GNU Coding Standards style */ > > State a preference for the former. > > Signed-off-by: Peter Maydell > --- > I admit that to some extent I'm imposing my aesthetic > preferences here; pretty sure we have a lot more style > 1 comments than style 2, though. > --- > CODING_STYLE | 13 + > 1 file changed, 13 insertions(+) > > diff --git a/CODING_STYLE b/CODING_STYLE > index 12ba58ee293..fb1d1f1cd62 100644 > --- a/CODING_STYLE > +++ b/CODING_STYLE > @@ -124,6 +124,19 @@ We use traditional C-style /* */ comments and avoid // > comments. > Rationale: The // form is valid in C99, so this is purely a matter of > consistency of style. The checkpatch script will warn you about this. > > +Multiline comments blocks should have a row of stars on the left > +and the terminating */ on its own line: > +/* like > + * this > + */ > +Putting the initial /* on its own line is accepted, but not required. > +(Some of the existing comments in the codebase use the GNU Coding > +Standards form which does not have stars on the left; avoid this > +when writing new comments.) > + > +Rationale: Consistency, and ease of visually picking out a multiline > +comment from the surrounding code. > + > 8. trace-events style > > 8.1 0x prefix > Is there some name for the unholy abomination that is the combination of both styles?: /* Like this, but without the trailing * end-comment on a standalone line. */ I prefer the asterisks on the left because it makes it obvious when grepping that it's from a comment block; I dislike the standalone comment for taking up a bunch of room for seemingly no reason. ...Not that I expect to change your mind, or to suggest I've got enough paint for a shed this large, so any standard is better than no standard... --js
Re: [Qemu-devel] [PATCH v7 0/7] aspeed: add a witherspoon-bmc machine
On 30 May 2018 at 07:40, Cédric Le Goater wrote: > Hello > > This series adds a new Aspeed machine to emulate the BMC of a > Witherspoon system. It also extends the other Aspeed machines with I2C > devices and adds a simple model for the pca9552 LED blinker present on > the Witherspoon board. > > Thanks, > > C. Applied to target-arm.next, thanks. -- PMM
Re: [Qemu-devel] [PATCH 2/6] ide: push end_transfer_func out of start_transfer callback, rename callback
On 06/04/2018 11:48 AM, Paolo Bonzini wrote:
> On 02/06/2018 02:24, John Snow wrote:
>>> -if (s->bus->dma->ops->start_transfer) {
>>> -s->bus->dma->ops->start_transfer(s->bus->dma);
>>> +if (!s->bus->dma->ops->pio_transfer) {
>>> +s->end_transfer_func = end_transfer_func;
>>> +return;
>>> }
>>> +s->bus->dma->ops->pio_transfer(s->bus->dma);
>>> +end_transfer_func(s);
>> Does not setting s->end_transfer_func mess with some of our dumb hacks
>> in e.g. ide_restart_bh or ide_is_pio_out?
>
> No, ide_is_pio_out is not used by AHCI and ide_restart_bh looks at the
> flags passed to ide_handle_rw_error.
>
> Thanks,
>
> Paolo
>
Yes, that's right -- ide_restart_bh relies less on this now (thanks to
you, if I recall correctly) and ide_is_pio_out is only used by
ide_data_(read|write)[lw] ...
Hmm, it'd be nice to get rid of our reliance on ETF to determine state,
but that's nothing you've caused.
Reviewed-by: John Snow
Re: [Qemu-devel] [PATCH v2 1/2] vl: don't use RUN_STATE_PRECONFIG as initial state
On Mon, 4 Jun 2018 17:11:57 +0100
Daniel P. Berrangé wrote:
> On Mon, Jun 04, 2018 at 05:40:32PM +0200, Igor Mammedov wrote:
> > On Mon, 4 Jun 2018 13:03:44 +0100
> > Daniel P. Berrangé wrote:
> >
> > > The RUN_STATE_PRECONFIG state is not supposed to be reachable unless the
> > > --preconfig argument is given to QEMU, but when it was introduced in:
> > >
> > > commit 047f7038f586d2150f16c6d9ba9cfd0479f0f6ac
> > > Author: Igor Mammedov
> > > Date: Fri May 11 19:24:43 2018 +0200
> > >
> > > cli: add --preconfig option
> > >
> > > ...the global 'current_run_state' variable was changed to have an initial
> > > value of RUN_STATE_PRECONFIG regardless of whether --preconfig is given.
> > >
> > > A second invokation of main_loop() was added which then toggles it back
> > > to RUN_STATE_PRELAUNCH when --preconfig is not given. This is racy
> > > because it leaves a window where QEMU is in RUN_STATE_PRECONFIG despite
> > > --preconfig not being given.
> > Above statements isn't exactly correct, PRECONFIG were supposed to be
> > the new state of QEMU from start up till machine_run_board_init(),
> > that would separate stage of initial configuration out from all
> > encompassing PRELAUNCH state. So I'd scratch out above part.
>
> That doesn't really make sense, given that --preconfig is *not* the
> default and thus not supposed to be an active state unless the app
> has explicitly opted in.
>
> IMHO running PRECONFIG state for any period of time when the app
> has not requested --preconfig is simply broken, and a recipe for
> obscure bugs like the ones we've seen right now.
mgmt hasn't opted in for default PRELAUNCH either, for it default
PRECONFIG runstate is not visible unless it opts in so nothing
is broken in regards to this.
Default runstate selection is QEMU's internal impl. detail.
I don't think it's the reason for obscure bugs, it's rather
exposing so far hidden issues out there. The current startup
code is a mess and were blindly assuming PRELAUNCH state,
using globals to jump from one state to another depending on
CLI options combination.
So moving main_loop() earlier exposed a number of issues,
that should be fixed.
> > > $ echo | x86_64-softmmu/qemu-system-x86_64 -monitor stdio
> > > QEMU 2.12.50 monitor - type 'help' for more information
> > > (qemu)
> > > HMP not available in preconfig state, use QMP instead
> > Michal's patch is much simpler and fixes that cleanly.
>
> It is incomplete IMHO as it still leaves the hang in main loop
> present - it merely avoids triggering it in one code path, and
> leaves the other code path broken.
It's new behavior which looses sync point on parent QEMU process exit,
hence it's not convenient for mgmt that currently expects parent
qemu process exit when it's demonized.
Since it's new option, there are 2 ways to deal with it:
* make parent process exit earlier before main loop as you suggest in 2/2
and teach mgmt to deal with initialization errors cleanly after
sync point
* teach mgmt to connect to QMP socket earlier if --preconfig were used,
(benefit error handling works as it used to be)
I'd happy with any of this as far as user won't be confused if something
goes wrong.
> > > Using RUN_STATE_PRECONFIG required adding a state transition from
> > > RUN_STATE_PRECONFIG to RUN_STATE_INMIGRATE, which is undesirable as
> > > it prevented automatic detection of --preconfig & --incoming being
> > > mutually exclusive.
> > >
> > > If we use RUN_STATE_PRELAUNCH as the initial value, however, we need to
> > > allow transitions between RUN_STATE_PRECONFIG & RUN_STATE_PRELAUNCH in
> > > both directions which is also undesirable, as RUN_STATE_PRECONFIG should
> > > be a one-time-only state that can never be returned to.
> > >
> > > This change solves the confusion by introducing a further RUN_STATE_NONE
> > > which is used as the initial state value. This can transition to any of
> > > RUN_STATE_PRELAUNCH, RUN_STATE_PRECONFIG or RUN_STATE_INMIGRATE. This
> > > avoids the need to allow any undesirable state transitions.
> > Ugly mutually exclusive code including related long comments are
> > intentional. The plan is to streamline runstate transitions in
> > following order
> > PRECONFIG -> PRELAUNCH [-> INMIGRATE]
>
> This doesn't make any conceptual sense to me, given that --preconfig
> is an optional flag. We can't make --preconfig the default, because
> of back compat, so we'll always have
where is the back compat issue with preconfig default?
(it's not visible to mgmt unless --preconfig option is used)
>$START -> PRELAUNCH {-> INMIGRATE]
> | ^
> | |
> +-- PRECONFIG -+
>
> By marking the current state as PRECONFIG by default, we've essentially
> given 2 meanings to PRECONFIG - sometimes it means stuff that can be
> unconditionally run in early startup, and sometimes it means stuff that
> can only be run if --preconfig is given. Introducing the
Re: [Qemu-devel] [PATCH v3b 12/18] target/arm: Implement SVE Integer Compare - Immediate Group
On 30 May 2018 at 19:01, Richard Henderson wrote: > Signed-off-by: Richard Henderson > --- > target/arm/helper-sve.h| 44 +++ > target/arm/sve_helper.c| 88 ++ > target/arm/translate-sve.c | 66 > target/arm/sve.decode | 23 ++ > 4 files changed, 221 insertions(+) Reviewed-by: Peter Maydell thanks -- PMM
Re: [Qemu-devel] [PULL 0/2] Vga 20180604 patches
On 4 June 2018 at 16:40, Peter Maydell wrote: > On 4 June 2018 at 09:49, Gerd Hoffmann wrote: >> The following changes since commit 392fba9f583223786f844dce9b2e7f9a0ce0147a: >> >> Merge remote-tracking branch >> 'remotes/stsquad/tags/pull-travis-updates-010618-1' into staging (2018-06-01 >> 17:32:30 +0100) >> >> are available in the git repository at: >> >> git://git.kraxel.org/qemu tags/vga-20180604-pull-request >> >> for you to fetch changes up to 6bc2fd57e1fc2d1957d1ff952793c53764130218: >> >> vga: cleanup surface handling (2018-06-04 09:44:10 +0200) >> >> >> Two little vga fixes. >> >> >> >> Gerd Hoffmann (2): >> bochs-display: add missing break >> vga: cleanup surface handling >> >> hw/display/bochs-display.c | 1 + >> hw/display/vga.c | 36 +++- >> 2 files changed, 20 insertions(+), 17 deletions(-) > > I got a failure in the migration tests with this applied: > Unfortunately this doesn't seem to reproduce. I haven't seen > it before, so my guess is this is an intermittent introduced > by your earlier migration pullreq which is now in master ? > Might only manifest when the host machine is under load or > during a 'make check -j8' parallel test run. A rerun didn't show this up, and it doesn't look like it could be anything in this pull, so I've applied the pullreq. thanks -- PMM
Re: [Qemu-devel] [PATCH v3b 11/18] target/arm: Implement SVE Integer Compare - Vectors Group
On 30 May 2018 at 19:01, Richard Henderson wrote: > Signed-off-by: Richard Henderson > --- > target/arm/helper-sve.h| 115 +++ > target/arm/sve_helper.c| 187 + > target/arm/translate-sve.c | 91 ++ > target/arm/sve.decode | 24 + > 4 files changed, 417 insertions(+) Reviewed-by: Peter Maydell thanks -- PMM
Re: [Qemu-devel] [PATCH v2] qcow2: add overlap check for bitmap directory
On 06/04/2018 12:11 PM, Vladimir Sementsov-Ogievskiy wrote:
> 20.04.2018 15:12, Vladimir Sementsov-Ogievskiy wrote:
>> 19.04.2018 23:57, John Snow wrote:
>>>
>>> On 03/19/2018 04:07 AM, Vladimir Sementsov-Ogievskiy wrote:
Signed-off-by: Vladimir Sementsov-Ogievskiy
---
If it appropriate for 2.12, let's push it. If not - then for 2.13.
>>> I wonder if I can make the case that this should be in 2.12.1; arguably
>>> it is important to prevent corruption no matter how unlikely it is to
>>> ever happen.
>>>
>>> Moving it into stable increases the likelihood it shows up in
>>> downstreams, so maybe let's see what we can get away with.
>>>
v2: - squash 02 (indentation fix) to 01
- drop comment from qcow2_check_metadata_overlap()
- set @ign to QCOW2_OL_BITMAP_DIRECTORY for in-place case in
bitmap_list_store. I don't think non-inplace case should be
changed,
as it don't touch active bitmap directory.
block/qcow2.h | 45
-
block/qcow2-bitmap.c | 7 ++-
block/qcow2-refcount.c | 10 ++
block/qcow2.c | 22 ++
4 files changed, 54 insertions(+), 30 deletions(-)
diff --git a/block/qcow2.h b/block/qcow2.h
index 6f0ff15dd0..896ad08e5b 100644
--- a/block/qcow2.h
+++ b/block/qcow2.h
@@ -98,6 +98,7 @@
#define QCOW2_OPT_OVERLAP_SNAPSHOT_TABLE
"overlap-check.snapshot-table"
#define QCOW2_OPT_OVERLAP_INACTIVE_L1 "overlap-check.inactive-l1"
#define QCOW2_OPT_OVERLAP_INACTIVE_L2 "overlap-check.inactive-l2"
+#define QCOW2_OPT_OVERLAP_BITMAP_DIRECTORY
"overlap-check.bitmap-directory"
#define QCOW2_OPT_CACHE_SIZE "cache-size"
#define QCOW2_OPT_L2_CACHE_SIZE "l2-cache-size"
#define QCOW2_OPT_REFCOUNT_CACHE_SIZE "refcount-cache-size"
@@ -398,34 +399,36 @@ typedef enum QCow2ClusterType {
} QCow2ClusterType;
typedef enum QCow2MetadataOverlap {
- QCOW2_OL_MAIN_HEADER_BITNR = 0,
- QCOW2_OL_ACTIVE_L1_BITNR = 1,
- QCOW2_OL_ACTIVE_L2_BITNR = 2,
- QCOW2_OL_REFCOUNT_TABLE_BITNR = 3,
- QCOW2_OL_REFCOUNT_BLOCK_BITNR = 4,
- QCOW2_OL_SNAPSHOT_TABLE_BITNR = 5,
- QCOW2_OL_INACTIVE_L1_BITNR = 6,
- QCOW2_OL_INACTIVE_L2_BITNR = 7,
-
- QCOW2_OL_MAX_BITNR = 8,
-
- QCOW2_OL_NONE = 0,
- QCOW2_OL_MAIN_HEADER = (1 << QCOW2_OL_MAIN_HEADER_BITNR),
- QCOW2_OL_ACTIVE_L1 = (1 << QCOW2_OL_ACTIVE_L1_BITNR),
- QCOW2_OL_ACTIVE_L2 = (1 << QCOW2_OL_ACTIVE_L2_BITNR),
- QCOW2_OL_REFCOUNT_TABLE = (1 << QCOW2_OL_REFCOUNT_TABLE_BITNR),
- QCOW2_OL_REFCOUNT_BLOCK = (1 << QCOW2_OL_REFCOUNT_BLOCK_BITNR),
- QCOW2_OL_SNAPSHOT_TABLE = (1 << QCOW2_OL_SNAPSHOT_TABLE_BITNR),
- QCOW2_OL_INACTIVE_L1 = (1 << QCOW2_OL_INACTIVE_L1_BITNR),
+ QCOW2_OL_MAIN_HEADER_BITNR = 0,
+ QCOW2_OL_ACTIVE_L1_BITNR = 1,
+ QCOW2_OL_ACTIVE_L2_BITNR = 2,
+ QCOW2_OL_REFCOUNT_TABLE_BITNR = 3,
+ QCOW2_OL_REFCOUNT_BLOCK_BITNR = 4,
+ QCOW2_OL_SNAPSHOT_TABLE_BITNR = 5,
+ QCOW2_OL_INACTIVE_L1_BITNR = 6,
+ QCOW2_OL_INACTIVE_L2_BITNR = 7,
+ QCOW2_OL_BITMAP_DIRECTORY_BITNR = 8,
+
>>> A bit hard to read due to the formatting, but you've added #8 here, and
>>>
+ QCOW2_OL_MAX_BITNR = 9,
+> + QCOW2_OL_NONE = 0,
+ QCOW2_OL_MAIN_HEADER = (1 << QCOW2_OL_MAIN_HEADER_BITNR),
+ QCOW2_OL_ACTIVE_L1 = (1 << QCOW2_OL_ACTIVE_L1_BITNR),
+ QCOW2_OL_ACTIVE_L2 = (1 << QCOW2_OL_ACTIVE_L2_BITNR),
+ QCOW2_OL_REFCOUNT_TABLE = (1 << QCOW2_OL_REFCOUNT_TABLE_BITNR),
+ QCOW2_OL_REFCOUNT_BLOCK = (1 << QCOW2_OL_REFCOUNT_BLOCK_BITNR),
+ QCOW2_OL_SNAPSHOT_TABLE = (1 << QCOW2_OL_SNAPSHOT_TABLE_BITNR),
+ QCOW2_OL_INACTIVE_L1 = (1 << QCOW2_OL_INACTIVE_L1_BITNR),
/* NOTE: Checking overlaps with inactive L2 tables will result
in bdrv
* reads. */
- QCOW2_OL_INACTIVE_L2 = (1 << QCOW2_OL_INACTIVE_L2_BITNR),
+ QCOW2_OL_INACTIVE_L2 = (1 << QCOW2_OL_INACTIVE_L2_BITNR),
+ QCOW2_OL_BITMAP_DIRECTORY = (1 <<
QCOW2_OL_BITMAP_DIRECTORY_BITNR),
>>> and this one down here.
>>>
} QCow2MetadataOverlap;
/* Perform all overlap checks which can be done in constant time */
#define QCOW2_OL_CONSTANT \
(QCOW2_OL_MAIN_HEADER | QCOW2_OL_ACTIVE_L1 |
QCOW2_OL_REFCOUNT_TABLE | \
- QCOW2_OL_SNAPSHOT_TABLE)
+ QCOW2_OL_SNAPSHOT_TABLE | QCOW2_OL_BITMAP_DIRECTORY)
/* Perform all overlap checks which don't require disk access */
#define QCOW2_OL_CACHED \
diff --git a/block/qcow2-bitmap.c
[Qemu-devel] [RFC PATCH] target/ppc: extend eieio for POWER9
POWER9 introduced a new variant of the eieio instruction using bit 6 as a hint to tell the CPU it is a store-forwarding barrier. The usage of this eieio extension was recently added in Linux 4.17 which activated the "support for a store forwarding barrier at kernel entry/exit". This loosen the QEMU eieio instruction mask to boot newer kernel but I think we should be adding a new *eieio* instruction specific to POWER9 instead. I just don't know how to define an instruction variant with the same op code for an ISA version. Any idea ? Signed-off-by: Cédric Le Goater --- target/ppc/translate.c |2 +- 1 file changed, 1 insertion(+), 1 deletion(-) Index: qemu-powernv-2.13.git/target/ppc/translate.c === --- qemu-powernv-2.13.git.orig/target/ppc/translate.c +++ qemu-powernv-2.13.git/target/ppc/translate.c @@ -6496,7 +6496,7 @@ GEN_HANDLER(lswi, 0x1F, 0x15, 0x12, 0x00 GEN_HANDLER(lswx, 0x1F, 0x15, 0x10, 0x0001, PPC_STRING), GEN_HANDLER(stswi, 0x1F, 0x15, 0x16, 0x0001, PPC_STRING), GEN_HANDLER(stswx, 0x1F, 0x15, 0x14, 0x0001, PPC_STRING), -GEN_HANDLER(eieio, 0x1F, 0x16, 0x1A, 0x03FFF801, PPC_MEM_EIEIO), +GEN_HANDLER(eieio, 0x1F, 0x16, 0x1A, 0x01FFF801, PPC_MEM_EIEIO), GEN_HANDLER(isync, 0x13, 0x16, 0x04, 0x03FFF801, PPC_MEM), GEN_HANDLER_E(lbarx, 0x1F, 0x14, 0x01, 0, PPC_NONE, PPC2_ATOMIC_ISA206), GEN_HANDLER_E(lharx, 0x1F, 0x14, 0x03, 0, PPC_NONE, PPC2_ATOMIC_ISA206),
Re: [Qemu-devel] [PATCH RFC] hw/pc: set q35 as the default x86 machine
On Mon, Jun 04, 2018 at 10:26:24AM -0300, Eduardo Habkost wrote: > On Mon, Jun 04, 2018 at 02:01:29PM +0100, Daniel P. Berrangé wrote: > > On Mon, Jun 04, 2018 at 09:54:15AM -0300, Eduardo Habkost wrote: > > > On Mon, Jun 04, 2018 at 04:38:22AM +0300, Michael S. Tsirkin wrote: > > > > On Sun, Jun 03, 2018 at 12:27:49PM +0300, Marcel Apfelbaum wrote: > > > > > Moving to QEMU 3.0 seems like a good opportunity for such a change. > > > > > > > > > > I440FX is really old and does not support modern features like IOMMU. > > > > > Q35's SATA emulation is faster than pc's IDE, native PCI express > > > > > hotplug > > > > > is cleaner than ACPI based one and so on... > > > > > > > > > > Also the libvirt guys added very good support for the Q35 machine > > > > > (thanks!). > > > > > > > > > > Management software should always specify the machine type and for the > > > > > current setups, adding '-machine pc' to the command line is not such a > > > > > big deal. > > > > > > > > > > In time the pc machine will fade out and we will probably stop adding > > > > > new versions at some point. > > > > > > > > > > Signed-off-by: Marcel Apfelbaum > > > > > > > > For command line users, I think changing the default isn't nice. > > > > > > > > Yes it's easy to add -machine pc but there's no documentation > > > > that tells you to do so. Add to that shortcuts like -cdrom > > > > stop working, hotplug needs extra bridges to work, and one > > > > can see that while management tool users benefit from q35, > > > > command line users will suffer. > > > > > > > > Can't we add a tag for management without changing the command line > > > > default? How about "management-default"? "recommended"? "latest"? > > > > > > We could add new aliases if they are useful for management > > > software, but we would need a well-defined use case and set of > > > requirements+expectations for the new alias. > > > > I'm not convinced by the idea of adding a distinct default "for mgmt". All > > the problems described wrt 'q35' vs 'pc' apply equally to management apps > > as they do to humans. It just happens that one common mgmt layer (libvirt) > > knows how to handle some of the complexity of q35. Other mgmt apps though > > are just as likely to be hurt by the change as humans are. So effectively > > the proposed "for mgmt" is actually "for libvirt >= some version", which > > feels like a layering violation to me. > > This means the new alias would be used only if requested > explicitly by management software (not used automatically by > libvirt). > > Taking that into account, I still don't see what exactly would be > the use case here, and what exactly users can/can't expect when > using the new alias. Let's see what we have now first: 1. We have a requirement for the user to save the machine type on install and maintain it with the image (a separate thread discusses saving that as part of a qcow2 image). 2. If you use an alias instead you are supposed to resolve it and save the resolved value. If you save the alias instead, you can't do cross-version live migration. 3. If you don't specify anything you get a machine tagged default. You are supposed to find it and save the value found. If you don't and just keep using the default, you can't do cross-version live migration. --- So now we would like to relax 3 to say "If you don't and just keep using the default, some images might not boot". unfortunately we probably can't change 3 like this. So what I propose instead is simply: 4. If you find a machine type value tagged "qmp-default" you must save the value found. If you don't and just keep using the qmp-default each time, then existing guest images won't boot. This relaxed compatibility requirement allows for advanced features as compared to default. > -- > Eduardo
Re: [Qemu-devel] [PATCH v3b 10/18] target/arm: Implement SVE Select Vectors Group
On 30 May 2018 at 19:01, Richard Henderson wrote: > Signed-off-by: Richard Henderson > --- > target/arm/helper-sve.h| 9 +++ > target/arm/sve_helper.c| 55 ++ > target/arm/translate-sve.c | 2 ++ > target/arm/sve.decode | 6 + > 4 files changed, 72 insertions(+) Reviewed-by: Peter Maydell thanks -- PMM
Re: [Qemu-devel] [PATCH v3b 09/18] target/arm: Implement SVE vector splice (predicated)
On 30 May 2018 at 19:01, Richard Henderson wrote: > Signed-off-by: Richard Henderson > --- > target/arm/helper-sve.h| 2 ++ > target/arm/sve_helper.c| 37 + > target/arm/translate-sve.c | 13 + > target/arm/sve.decode | 3 +++ > 4 files changed, 55 insertions(+) Reviewed-by: Peter Maydell thanks -- PMM
Re: [Qemu-devel] [PATCH v3b 08/18] target/arm: Implement SVE reverse within elements
On 30 May 2018 at 19:01, Richard Henderson wrote:
> Signed-off-by: Richard Henderson
> ---
> target/arm/helper-sve.h| 14 +
> target/arm/sve_helper.c| 41 +++---
> target/arm/translate-sve.c | 38 +++
> target/arm/sve.decode | 7 +++
> 4 files changed, 93 insertions(+), 7 deletions(-)
> diff --git a/target/arm/sve_helper.c b/target/arm/sve_helper.c
> index 941a098234..f8579a25e3 100644
> --- a/target/arm/sve_helper.c
> +++ b/target/arm/sve_helper.c
> @@ -238,6 +238,26 @@ static inline uint64_t expand_pred_s(uint8_t byte)
> return word[byte & 0x11];
> }
>
> +/* Swap 16-bit words within a 32-bit word. */
> +static inline uint32_t hswap32(uint32_t h)
> +{
> +return rol32(h, 16);
> +}
> +
> +/* Swap 16-bit words within a 64-bit word. */
> +static inline uint64_t hswap64(uint64_t h)
> +{
> +uint64_t m = 0xull;
> +h = rol64(h, 32);
> +return ((h & m) << 16) | ((h >> 16) & m);
> +}
> +
> +/* Swap 32-bit words within a 64-bit word. */
> +static inline uint64_t wswap64(uint64_t h)
> +{
> +return rol64(h, 32);
> +}
> +
> #define LOGICAL_(NAME, FUNC) \
> void HELPER(NAME)(void *vd, void *vn, void *vm, void *vg, uint32_t desc) \
> { \
> @@ -616,6 +636,20 @@ DO_ZPZ(sve_neg_h, uint16_t, H1_2, DO_NEG)
> DO_ZPZ(sve_neg_s, uint32_t, H1_4, DO_NEG)
> DO_ZPZ_D(sve_neg_d, uint64_t, DO_NEG)
>
> +DO_ZPZ(sve_revb_h, uint16_t, H1_2, bswap16)
> +DO_ZPZ(sve_revb_s, uint32_t, H1_4, bswap32)
> +DO_ZPZ_D(sve_revb_d, uint64_t, bswap64)
> +
> +DO_ZPZ(sve_revh_s, uint32_t, H1_4, hswap32)
> +DO_ZPZ_D(sve_revh_d, uint64_t, hswap64)
> +
> +DO_ZPZ_D(sve_revw_d, uint64_t, wswap64)
> +
> +DO_ZPZ(sve_rbit_b, uint8_t, H1, revbit8)
> +DO_ZPZ(sve_rbit_h, uint16_t, H1_2, revbit16)
> +DO_ZPZ(sve_rbit_s, uint32_t, H1_4, revbit32)
> +DO_ZPZ_D(sve_rbit_d, uint64_t, revbit64)
> +
> /* Three-operand expander, unpredicated, in which the third operand is
> "wide".
> */
> #define DO_ZZW(NAME, TYPE, TYPEW, H, OP) \
> @@ -1587,13 +1621,6 @@ void HELPER(sve_rev_b)(void *vd, void *vn, uint32_t
> desc)
> }
> }
>
> -static inline uint64_t hswap64(uint64_t h)
> -{
> -uint64_t m = 0xull;
> -h = rol64(h, 32);
> -return ((h & m) << 16) | ((h >> 16) & m);
> -}
> -
You added this in patch 2, I think -- you could avoid the code
motion here by putting it in the right place to start with.
> void HELPER(sve_rev_h)(void *vd, void *vn, uint32_t desc)
> {
> intptr_t i, j, opr_sz = simd_oprsz(desc);
Otherwise
Reviewed-by: Peter Maydell
thanks
-- PMM
Re: [Qemu-devel] [PATCH RFC] hw/pc: set q35 as the default x86 machine
On Mon, Jun 04, 2018 at 07:48:51PM +0300, Michael S. Tsirkin wrote: > On Mon, Jun 04, 2018 at 02:01:29PM +0100, Daniel P. Berrangé wrote: > > On Mon, Jun 04, 2018 at 09:54:15AM -0300, Eduardo Habkost wrote: > > > On Mon, Jun 04, 2018 at 04:38:22AM +0300, Michael S. Tsirkin wrote: > > > > On Sun, Jun 03, 2018 at 12:27:49PM +0300, Marcel Apfelbaum wrote: > > > > > Moving to QEMU 3.0 seems like a good opportunity for such a change. > > > > > > > > > > I440FX is really old and does not support modern features like IOMMU. > > > > > Q35's SATA emulation is faster than pc's IDE, native PCI express > > > > > hotplug > > > > > is cleaner than ACPI based one and so on... > > > > > > > > > > Also the libvirt guys added very good support for the Q35 machine > > > > > (thanks!). > > > > > > > > > > Management software should always specify the machine type and for the > > > > > current setups, adding '-machine pc' to the command line is not such a > > > > > big deal. > > > > > > > > > > In time the pc machine will fade out and we will probably stop adding > > > > > new versions at some point. > > > > > > > > > > Signed-off-by: Marcel Apfelbaum > > > > > > > > For command line users, I think changing the default isn't nice. > > > > > > > > Yes it's easy to add -machine pc but there's no documentation > > > > that tells you to do so. Add to that shortcuts like -cdrom > > > > stop working, hotplug needs extra bridges to work, and one > > > > can see that while management tool users benefit from q35, > > > > command line users will suffer. > > > > > > > > Can't we add a tag for management without changing the command line > > > > default? How about "management-default"? "recommended"? "latest"? > > > > > > We could add new aliases if they are useful for management > > > software, but we would need a well-defined use case and set of > > > requirements+expectations for the new alias. > > > > I'm not convinced by the idea of adding a distinct default "for mgmt". All > > the problems described wrt 'q35' vs 'pc' apply equally to management apps > > as they do to humans. It just happens that one common mgmt layer (libvirt) > > knows how to handle some of the complexity of q35. Other mgmt apps though > > are just as likely to be hurt by the change as humans are. So effectively > > the proposed "for mgmt" is actually "for libvirt >= some version", which > > feels like a layering violation to me. > > Is libvirt happy to just hard-code q35 for now then? I'm pretty wary of doing that, as I feel 'pc' has broader OS compatibility than 'q35', so we'd be likely to cause breakage for users. IMHO, defaults are something better expressed in libosinfo, so if there's guests where we think q35 is a better choice, we can record it there and leave everything else on pc to avoid risk of breakage. 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 v3b 07/18] target/arm: Implement SVE copy to vector (predicated)
On 30 May 2018 at 19:01, Richard Henderson wrote: > Signed-off-by: Richard Henderson > --- > target/arm/translate-sve.c | 19 +++ > target/arm/sve.decode | 6 ++ > 2 files changed, 25 insertions(+) Reviewed-by: Peter Maydell thanks -- PMM
Re: [Qemu-devel] [PATCH v3b 01/18] target/arm: Extend vec_reg_offset to larger sizes
On 30 May 2018 at 19:01, Richard Henderson wrote: > Rearrange the arithmetic so that we are agnostic about the total size > of the vector and the size of the element. This will allow us to index > up to the 32nd byte and with 16-byte elements. > > Signed-off-by: Richard Henderson > --- > target/arm/translate-a64.h | 26 +- > 1 file changed, 17 insertions(+), 9 deletions(-) > Reviewed-by: Peter Maydell thanks -- PMM
Re: [Qemu-devel] [PATCH RFC] hw/pc: set q35 as the default x86 machine
On Mon, Jun 04, 2018 at 02:01:29PM +0100, Daniel P. Berrangé wrote: > On Mon, Jun 04, 2018 at 09:54:15AM -0300, Eduardo Habkost wrote: > > On Mon, Jun 04, 2018 at 04:38:22AM +0300, Michael S. Tsirkin wrote: > > > On Sun, Jun 03, 2018 at 12:27:49PM +0300, Marcel Apfelbaum wrote: > > > > Moving to QEMU 3.0 seems like a good opportunity for such a change. > > > > > > > > I440FX is really old and does not support modern features like IOMMU. > > > > Q35's SATA emulation is faster than pc's IDE, native PCI express hotplug > > > > is cleaner than ACPI based one and so on... > > > > > > > > Also the libvirt guys added very good support for the Q35 machine > > > > (thanks!). > > > > > > > > Management software should always specify the machine type and for the > > > > current setups, adding '-machine pc' to the command line is not such a > > > > big deal. > > > > > > > > In time the pc machine will fade out and we will probably stop adding > > > > new versions at some point. > > > > > > > > Signed-off-by: Marcel Apfelbaum > > > > > > For command line users, I think changing the default isn't nice. > > > > > > Yes it's easy to add -machine pc but there's no documentation > > > that tells you to do so. Add to that shortcuts like -cdrom > > > stop working, hotplug needs extra bridges to work, and one > > > can see that while management tool users benefit from q35, > > > command line users will suffer. > > > > > > Can't we add a tag for management without changing the command line > > > default? How about "management-default"? "recommended"? "latest"? > > > > We could add new aliases if they are useful for management > > software, but we would need a well-defined use case and set of > > requirements+expectations for the new alias. > > I'm not convinced by the idea of adding a distinct default "for mgmt". All > the problems described wrt 'q35' vs 'pc' apply equally to management apps > as they do to humans. It just happens that one common mgmt layer (libvirt) > knows how to handle some of the complexity of q35. Other mgmt apps though > are just as likely to be hurt by the change as humans are. So effectively > the proposed "for mgmt" is actually "for libvirt >= some version", which > feels like a layering violation to me. > > Regards, > Daniel Is libvirt happy to just hard-code q35 for now then? > -- > |: 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 v3b 06/18] target/arm: Implement SVE conditionally broadcast/extract element
On 30 May 2018 at 19:01, Richard Henderson wrote:
> Signed-off-by: Richard Henderson
> ---
> target/arm/helper-sve.h| 2 +
> target/arm/sve_helper.c| 11 ++
> target/arm/translate-sve.c | 318 +
> target/arm/sve.decode | 20 +++
> 4 files changed, 351 insertions(+)
>
> diff --git a/target/arm/helper-sve.h b/target/arm/helper-sve.h
> index d977aea00d..a58fb4ba01 100644
> --- a/target/arm/helper-sve.h
> +++ b/target/arm/helper-sve.h
> @@ -463,6 +463,8 @@ DEF_HELPER_FLAGS_4(sve_trn_d, TCG_CALL_NO_RWG, void, ptr,
> ptr, ptr, i32)
> DEF_HELPER_FLAGS_4(sve_compact_s, TCG_CALL_NO_RWG, void, ptr, ptr, ptr, i32)
> DEF_HELPER_FLAGS_4(sve_compact_d, TCG_CALL_NO_RWG, void, ptr, ptr, ptr, i32)
>
> +DEF_HELPER_FLAGS_2(sve_last_active_element, TCG_CALL_NO_RWG, s32, ptr, i32)
> +
> DEF_HELPER_FLAGS_5(sve_and_, TCG_CALL_NO_RWG, void, ptr, ptr, ptr, ptr,
> i32)
> DEF_HELPER_FLAGS_5(sve_bic_, TCG_CALL_NO_RWG, void, ptr, ptr, ptr, ptr,
> i32)
> DEF_HELPER_FLAGS_5(sve_eor_, TCG_CALL_NO_RWG, void, ptr, ptr, ptr, ptr,
> i32)
> diff --git a/target/arm/sve_helper.c b/target/arm/sve_helper.c
> index ed3c6d4ca9..941a098234 100644
> --- a/target/arm/sve_helper.c
> +++ b/target/arm/sve_helper.c
> @@ -2070,3 +2070,14 @@ void HELPER(sve_compact_d)(void *vd, void *vn, void
> *vg, uint32_t desc)
> d[j] = 0;
> }
> }
> +
> +/* Similar to the ARM LastActiveElement pseudocode function, except the
> + result is multiplied by the element size. This includes the not found
> + indication; e.g. not found for esz=3 is -8. */
Non-standard multiline comment form... (I won't bother to note
this in other patches, but please check those too.)
> +int32_t HELPER(sve_last_active_element)(void *vg, uint32_t pred_desc)
> +{
> +intptr_t oprsz = extract32(pred_desc, 0, SIMD_OPRSZ_BITS) + 2;
> +intptr_t esz = extract32(pred_desc, SIMD_DATA_SHIFT, 2);
> +
> +return last_active_element(vg, DIV_ROUND_UP(oprsz, 8), esz);
> +}
> diff --git a/target/arm/translate-sve.c b/target/arm/translate-sve.c
> index ed0f48a927..edcef277f8 100644
> +/* Compute CLAST for a Zreg. */
> +static bool do_clast_vector(DisasContext *s, arg_rprr_esz *a, bool before)
> +{
> +if (!sve_access_check(s)) {
> +return true;
> +}
> +
> +TCGv_i32 last = tcg_temp_local_new_i32();
> +TCGLabel *over = gen_new_label();
> +TCGv_i64 ele;
> +unsigned vsz, esz = a->esz;
Declarations should be at the start of the block. (There's
another instance of this for the sve_access_check() in a
function below; please check other patches too.)
> +/* Compute CLAST for a Xreg. */
> +static bool do_clast_general(DisasContext *s, arg_rpr_esz *a, bool before)
> +{
> +if (!sve_access_check(s)) {
> +return true;
> +}
> +
> +TCGv_i64 reg = cpu_reg(s, a->rd);
> +
> +switch (a->esz) {
> +case 0:
> +tcg_gen_ext8u_i64(reg, reg);
> +break;
> +case 1:
> +tcg_gen_ext16u_i64(reg, reg);
> +break;
> +case 2:
> +tcg_gen_ext32u_i64(reg, reg);
> +break;
> +case 3:
> +break;
> +default:
> +g_assert_not_reached();
> +}
> +
> +do_clast_scalar(s, a->esz, a->pg, a->rn, before, cpu_reg(s, a->rd));
Why do we use cpu_reg() again here rather than just using 'reg' ?
> +return true;
> +}
> +
Otherwise
Reviewed-by: Peter Maydell
thanks
-- PMM
Re: [Qemu-devel] [PATCH v2 00/13] iommu: support txattrs, support TCG execution, implement TZ MPC
Hi,
This series seems to have some coding style problems. See output below for
more information:
Type: series
Message-id: 20180604152941.20374-1-peter.mayd...@linaro.org
Subject: [Qemu-devel] [PATCH v2 00/13] iommu: support txattrs, support TCG
execution, implement TZ MPC
=== TEST SCRIPT BEGIN ===
#!/bin/bash
BASE=base
n=1
total=$(git log --oneline $BASE.. | wc -l)
failed=0
git config --local diff.renamelimit 0
git config --local diff.renames True
git config --local diff.algorithm histogram
commits="$(git log --format=%H --reverse $BASE..)"
for c in $commits; do
echo "Checking PATCH $n/$total: $(git log -n 1 --format=%s $c)..."
if ! git show $c --format=email | ./scripts/checkpatch.pl --mailback -; then
failed=1
echo
fi
n=$((n+1))
done
exit $failed
=== TEST SCRIPT END ===
Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
Switched to a new branch 'test'
3b4bfa3eff hw/arm/mps2-tz.c: Instantiate MPCs
c2790822f9 hw/arm/iotkit: Wire up MPC interrupt lines
9746b202df hw/arm/iotkit: Instantiate MPC
4d48d06522 hw/misc/iotkit-secctl.c: Implement SECMPCINTSTATUS
5e08d07dc5 hw/core/or-irq: Support more than 16 inputs to an OR gate
dff0cc68f1 hw/misc/tz_mpc.c: Honour the BLK_LUT settings in translate
cc42636d7c hw/misc/tz-mpc.c: Implement correct blocked-access behaviour
57b4be59f6 hw/misc/tz-mpc.c: Implement registers
cb3c936f45 hw/misc/tz-mpc.c: Implement the Arm TrustZone Memory Protection
Controller
7d9ec3d9dd exec.c: Handle IOMMUs in address_space_translate_for_iotlb()
3c592827bc iommu: Add IOMMU index argument to translate method
ca96fcdcf6 iommu: Add IOMMU index argument to notifier APIs
85bade9641 iommu: Add IOMMU index concept to IOMMU API
=== OUTPUT BEGIN ===
Checking PATCH 1/13: iommu: Add IOMMU index concept to IOMMU API...
Checking PATCH 2/13: iommu: Add IOMMU index argument to notifier APIs...
Checking PATCH 3/13: iommu: Add IOMMU index argument to translate method...
Checking PATCH 4/13: exec.c: Handle IOMMUs in
address_space_translate_for_iotlb()...
Checking PATCH 5/13: hw/misc/tz-mpc.c: Implement the Arm TrustZone Memory
Protection Controller...
WARNING: added, moved or deleted file(s), does MAINTAINERS need updating?
#84:
new file mode 100644
total: 0 errors, 1 warnings, 486 lines checked
Your patch has style problems, please review. If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.
Checking PATCH 6/13: hw/misc/tz-mpc.c: Implement registers...
Checking PATCH 7/13: hw/misc/tz-mpc.c: Implement correct blocked-access
behaviour...
Checking PATCH 8/13: hw/misc/tz_mpc.c: Honour the BLK_LUT settings in
translate...
Checking PATCH 9/13: hw/core/or-irq: Support more than 16 inputs to an OR
gate...
ERROR: spaces required around that '*' (ctx:VxV)
#70: FILE: hw/core/or-irq.c:108:
+.subsections = (const VMStateDescription*[]) {
^
total: 1 errors, 0 warnings, 62 lines checked
Your patch has style problems, please review. If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.
Checking PATCH 10/13: hw/misc/iotkit-secctl.c: Implement SECMPCINTSTATUS...
ERROR: spaces required around that '*' (ctx:VxV)
#91: FILE: hw/misc/iotkit-secctl.c:711:
+.subsections = (const VMStateDescription*[]) {
^
total: 1 errors, 0 warnings, 100 lines checked
Your patch has style problems, please review. If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.
Checking PATCH 11/13: hw/arm/iotkit: Instantiate MPC...
Checking PATCH 12/13: hw/arm/iotkit: Wire up MPC interrupt lines...
Checking PATCH 13/13: hw/arm/mps2-tz.c: Instantiate MPCs...
=== OUTPUT END ===
Test command exited with code: 1
---
Email generated automatically by Patchew [http://patchew.org/].
Please send your feedback to patchew-de...@redhat.com
[Qemu-devel] [PATCH] CODING_STYLE: Define our preferred form for multiline comments
The codebase has a bit of a mix of /* multiline comments * like this */ and /* multiline comments like this in the GNU Coding Standards style */ State a preference for the former. Signed-off-by: Peter Maydell --- I admit that to some extent I'm imposing my aesthetic preferences here; pretty sure we have a lot more style 1 comments than style 2, though. --- CODING_STYLE | 13 + 1 file changed, 13 insertions(+) diff --git a/CODING_STYLE b/CODING_STYLE index 12ba58ee293..fb1d1f1cd62 100644 --- a/CODING_STYLE +++ b/CODING_STYLE @@ -124,6 +124,19 @@ We use traditional C-style /* */ comments and avoid // comments. Rationale: The // form is valid in C99, so this is purely a matter of consistency of style. The checkpatch script will warn you about this. +Multiline comments blocks should have a row of stars on the left +and the terminating */ on its own line: +/* like + * this + */ +Putting the initial /* on its own line is accepted, but not required. +(Some of the existing comments in the codebase use the GNU Coding +Standards form which does not have stars on the left; avoid this +when writing new comments.) + +Rationale: Consistency, and ease of visually picking out a multiline +comment from the surrounding code. + 8. trace-events style 8.1 0x prefix -- 2.17.1
Re: [Qemu-devel] [PATCH v2 1/2] vl: don't use RUN_STATE_PRECONFIG as initial state
On Mon, Jun 04, 2018 at 05:40:32PM +0200, Igor Mammedov wrote:
> On Mon, 4 Jun 2018 13:03:44 +0100
> Daniel P. Berrangé wrote:
>
> > The RUN_STATE_PRECONFIG state is not supposed to be reachable unless the
> > --preconfig argument is given to QEMU, but when it was introduced in:
> >
> > commit 047f7038f586d2150f16c6d9ba9cfd0479f0f6ac
> > Author: Igor Mammedov
> > Date: Fri May 11 19:24:43 2018 +0200
> >
> > cli: add --preconfig option
> >
> > ...the global 'current_run_state' variable was changed to have an initial
> > value of RUN_STATE_PRECONFIG regardless of whether --preconfig is given.
> >
> > A second invokation of main_loop() was added which then toggles it back
> > to RUN_STATE_PRELAUNCH when --preconfig is not given. This is racy
> > because it leaves a window where QEMU is in RUN_STATE_PRECONFIG despite
> > --preconfig not being given.
> Above statements isn't exactly correct, PRECONFIG were supposed to be
> the new state of QEMU from start up till machine_run_board_init(),
> that would separate stage of initial configuration out from all
> encompassing PRELAUNCH state. So I'd scratch out above part.
That doesn't really make sense, given that --preconfig is *not* the
default and thus not supposed to be an active state unless the app
has explicitly opted in.
IMHO running PRECONFIG state for any period of time when the app
has not requested --preconfig is simply broken, and a recipe for
obscure bugs like the ones we've seen right now.
> > $ echo | x86_64-softmmu/qemu-system-x86_64 -monitor stdio
> > QEMU 2.12.50 monitor - type 'help' for more information
> > (qemu)
> > HMP not available in preconfig state, use QMP instead
> Michal's patch is much simpler and fixes that cleanly.
It is incomplete IMHO as it still leaves the hang in main loop
present - it merely avoids triggering it in one code path, and
leaves the other code path broken.
> > Using RUN_STATE_PRECONFIG required adding a state transition from
> > RUN_STATE_PRECONFIG to RUN_STATE_INMIGRATE, which is undesirable as
> > it prevented automatic detection of --preconfig & --incoming being
> > mutually exclusive.
> >
> > If we use RUN_STATE_PRELAUNCH as the initial value, however, we need to
> > allow transitions between RUN_STATE_PRECONFIG & RUN_STATE_PRELAUNCH in
> > both directions which is also undesirable, as RUN_STATE_PRECONFIG should
> > be a one-time-only state that can never be returned to.
> >
> > This change solves the confusion by introducing a further RUN_STATE_NONE
> > which is used as the initial state value. This can transition to any of
> > RUN_STATE_PRELAUNCH, RUN_STATE_PRECONFIG or RUN_STATE_INMIGRATE. This
> > avoids the need to allow any undesirable state transitions.
> Ugly mutually exclusive code including related long comments are
> intentional. The plan is to streamline runstate transitions in
> following order
> PRECONFIG -> PRELAUNCH [-> INMIGRATE]
This doesn't make any conceptual sense to me, given that --preconfig
is an optional flag. We can't make --preconfig the default, because
of back compat, so we'll always have
$START -> PRELAUNCH {-> INMIGRATE]
| ^
| |
+-- PRECONFIG -+
By marking the current state as PRECONFIG by default, we've essentially
given 2 meanings to PRECONFIG - sometimes it means stuff that can be
unconditionally run in early startup, and sometimes it means stuff that
can only be run if --preconfig is given. Introducing the separate NONE
state clarifies that inherant contradiction in startup phases.
> i.e. postpone RUN_STATE_INMIGRATE transition to the later stage.
> But that's requires some cleanup work to remove invariants
> in initialization depending if INMIGRATE state is in effect or not.
>
> Hence I'd keep this part ugly as it is for now, and if we can do
> without RUN_STATE_NONE it would be better, i.e. keep current
> PRECONFIG runstate meaning where we would do initial configuration
> either via CLI or via QMP and one less runstate to deal with.
>
> I'd go with Michal's version of fix and think some more on
> introducing RUN_STATE_NONE.
>
> > Signed-off-by: Daniel P. Berrangé
> > ---
> > qapi/run-state.json | 6 +-
> > vl.c| 42 --
> > 2 files changed, 29 insertions(+), 19 deletions(-)
> >
> > diff --git a/qapi/run-state.json b/qapi/run-state.json
> > index 332e44897b..c3bd7b9b7a 100644
> > --- a/qapi/run-state.json
> > +++ b/qapi/run-state.json
> > @@ -10,6 +10,10 @@
> > #
> > # An enumeration of VM run states.
> > #
> > +# @none: QEMU is in early startup. This state should never be visible
> > +# when querying state from the monitor, as QEMU will have already
> > +# transitioned to another state. (Since 3.0)
> > +#
> > # @debug: QEMU is running on a debugger
> > #
> > # @finish-migrate: guest is paused to finish the migration process
> > @@ -54,7 +58,7 @@
> > # (Since 3.0)
> > ##
> > { 'enum':
