Re: [PATCH] audio/jack: add JACK client audiodev
Patchew URL:
https://patchew.org/QEMU/20200429061054.348b23c0...@aeryn.lan.ktmba/
Hi,
This series seems to have some coding style problems. See output below for
more information:
Subject: [PATCH] audio/jack: add JACK client audiodev
Message-id: 20200429061054.348b23c0...@aeryn.lan.ktmba
Type: series
=== TEST SCRIPT BEGIN ===
#!/bin/bash
git rev-parse base > /dev/null || exit 0
git config --local diff.renamelimit 0
git config --local diff.renames True
git config --local diff.algorithm histogram
./scripts/checkpatch.pl --mailback base..
=== TEST SCRIPT END ===
Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
From https://github.com/patchew-project/qemu
- [tag update] patchew/20200428172634.29707-1-f4...@amsat.org ->
patchew/20200428172634.29707-1-f4...@amsat.org
* [new tag] patchew/20200429061039.12687-1-vsement...@virtuozzo.com ->
patchew/20200429061039.12687-1-vsement...@virtuozzo.com
Switched to a new branch 'test'
aaf9cf2 audio/jack: add JACK client audiodev
=== OUTPUT BEGIN ===
WARNING: added, moved or deleted file(s), does MAINTAINERS need updating?
#52:
new file mode 100644
ERROR: open brace '{' following enum go on the same line
#97: FILE: audio/jackaudio.c:41:
+typedef enum QJackState
+{
ERROR: open brace '{' following struct go on the same line
#108: FILE: audio/jackaudio.c:52:
+typedef struct QJackBuffer
+{
ERROR: "foo ** bar" should be "foo **bar"
#113: FILE: audio/jackaudio.c:57:
+ float ** data;
ERROR: open brace '{' following struct go on the same line
#118: FILE: audio/jackaudio.c:62:
+typedef struct QJackClient
+{
ERROR: "foo * bar" should be "foo *bar"
#121: FILE: audio/jackaudio.c:65:
+ jack_client_t * client;
ERROR: "foo * bar" should be "foo *bar"
#124: FILE: audio/jackaudio.c:68:
+ struct QJack * j;
ERROR: "foo ** bar" should be "foo **bar"
#127: FILE: audio/jackaudio.c:71:
+ jack_port_t ** port;
ERROR: open brace '{' following struct go on the same line
#133: FILE: audio/jackaudio.c:77:
+typedef struct QJackOut
+{
ERROR: open brace '{' following struct go on the same line
#140: FILE: audio/jackaudio.c:84:
+typedef struct QJackIn
+{
ERROR: "foo * bar" should be "foo *bar"
#146: FILE: audio/jackaudio.c:90:
+static void qjack_buffer_create(QJackBuffer * buffer, int channels, int frames)
ERROR: space required before the open parenthesis '('
#154: FILE: audio/jackaudio.c:98:
+for(int i = 0; i < channels; ++i)
ERROR: braces {} are necessary for all arms of this statement
#154: FILE: audio/jackaudio.c:98:
+for(int i = 0; i < channels; ++i)
[...]
ERROR: "foo * bar" should be "foo *bar"
#158: FILE: audio/jackaudio.c:102:
+static void qjack_buffer_clear(QJackBuffer * buffer)
ERROR: "foo * bar" should be "foo *bar"
#165: FILE: audio/jackaudio.c:109:
+static void qjack_buffer_free(QJackBuffer * buffer)
ERROR: space required before the open parenthesis '('
#167: FILE: audio/jackaudio.c:111:
+for(int i = 0; i < buffer->channels; ++i)
ERROR: braces {} are necessary for all arms of this statement
#167: FILE: audio/jackaudio.c:111:
+for(int i = 0; i < buffer->channels; ++i)
[...]
ERROR: "foo * bar" should be "foo *bar"
#174: FILE: audio/jackaudio.c:118:
+inline static int qjack_buffer_used(QJackBuffer * buffer)
ERROR: storage class should be at the beginning of the declaration
#174: FILE: audio/jackaudio.c:118:
+inline static int qjack_buffer_used(QJackBuffer * buffer)
ERROR: inline keyword should sit between storage class and type
#174: FILE: audio/jackaudio.c:118:
+inline static int qjack_buffer_used(QJackBuffer * buffer)
ERROR: "foo * bar" should be "foo *bar"
#180: FILE: audio/jackaudio.c:124:
+static int qjack_buffer_write(QJackBuffer * buffer, float * data, int size)
ERROR: braces {} are necessary for all arms of this statement
#187: FILE: audio/jackaudio.c:131:
+if (frames > avail)
[...]
ERROR: space required before the open parenthesis '('
#193: FILE: audio/jackaudio.c:137:
+while(copy) {
ERROR: space required before the open parenthesis '('
#195: FILE: audio/jackaudio.c:139:
+for(int c = 0; c < buffer->channels; ++c)
ERROR: braces {} are necessary for all arms of this statement
#195: FILE: audio/jackaudio.c:139:
+for(int c = 0; c < buffer->channels; ++c)
[...]
ERROR: braces {} are necessary for all arms of this statement
#198: FILE: audio/jackaudio.c:142:
+if (++wptr == buffer->frames)
[...]
ERROR: "foo * bar" should be "foo *bar"
#211: FILE: audio/jackaudio.c:155:
+static int qjack_buffer_write_l(QJackBuffer * buffer, float ** dest, int
frames)
ERROR: braces {} are necessary for all arms of this statement
#217: FILE: audio/jackaudio.c:161:
+if (frames > avail)
[...]
ERROR: suspect code indent for conditional statements (4, 6)
#221: FILE: audio/jackaudio.c:165:
+if (right > frames)
+ right = frames;
ERROR: braces {} are necessary for all arms of this statement
#221: FILE: audio/jackaudio.c:165:
+if (right > frames)
[...]
ERROR: space require
Re: [PATCH] s390x/kvm: help valgrind in several places
Hi Christian,
On 4/28/20 8:31 PM, Christian Borntraeger wrote:
We need some little help in the code to reduce the valgrind noise.
- some designated initializers for the cpu model features and subfunctions
^ This could go as trivial patch while we discuss the rest.
- mark memory as defined for sida memory reads
Signed-off-by: Christian Borntraeger
---
I couldn't apply this patch, then figured out it targets s390-next.
target/s390x/kvm.c | 15 +--
1 file changed, 13 insertions(+), 2 deletions(-)
diff --git a/target/s390x/kvm.c b/target/s390x/kvm.c
index 69881a0da0..bcd0ee0d14 100644
--- a/target/s390x/kvm.c
+++ b/target/s390x/kvm.c
@@ -52,6 +52,10 @@
#include "hw/s390x/s390-virtio-hcall.h"
#include "hw/s390x/pv.h"
+#ifdef CONFIG_VALGRIND_H
+#include
+#endif
+
#ifndef DEBUG_KVM
#define DEBUG_KVM 0
#endif
@@ -875,6 +879,13 @@ int kvm_s390_mem_op_pv(S390CPU *cpu, uint64_t offset, void
*hostbuf,
error_report("KVM_S390_MEM_OP failed: %s", strerror(-ret));
abort();
}
What about kvm_s390_mem_op()?
+
+#ifdef CONFIG_VALGRIND_H
+if (!is_write) {
+VALGRIND_MAKE_MEM_DEFINED(hostbuf, len);
+}
+#endif
I agree with this macro usage, but think it should be widely accessible
by the whole codebase (and other targets).
"exec/memory.h" is for MemoryRegion and AddressSpace. Maybe
"exec/ram_addr.h" is a better place for common helpers.
If Valgrind is only confused under KVM, the "sysemu/kvm.h" is the
obvious place.
+
return ret;
}
@@ -2165,7 +2176,7 @@ int kvm_arch_msi_data_to_gsi(uint32_t data)
static int query_cpu_subfunc(S390FeatBitmap features)
{
-struct kvm_s390_vm_cpu_subfunc prop;
+struct kvm_s390_vm_cpu_subfunc prop = {};
struct kvm_device_attr attr = {
.group = KVM_S390_VM_CPU_MODEL,
.attr = KVM_S390_VM_CPU_MACHINE_SUBFUNC,
@@ -2292,7 +2303,7 @@ static int kvm_to_feat[][2] = {
static int query_cpu_feat(S390FeatBitmap features)
{
-struct kvm_s390_vm_cpu_feat prop;
+struct kvm_s390_vm_cpu_feat prop = {};
struct kvm_device_attr attr = {
.group = KVM_S390_VM_CPU_MODEL,
.attr = KVM_S390_VM_CPU_MACHINE_FEAT,
Re: [PATCH V2] Add a new PIIX option to control PCI hot unplugging of devices on non-root buses
> On Apr 29, 2020, at 12:27 PM, Michael S. Tsirkin wrote: > > On Wed, Apr 29, 2020 at 06:54:52AM +, Ani Sinha wrote: >> >> >>> On Apr 29, 2020, at 12:22 PM, Michael S. Tsirkin wrote: >>> >>> On Wed, Apr 29, 2020 at 06:11:20AM +, Ani Sinha wrote: > On Apr 29, 2020, at 10:58 AM, Michael S. Tsirkin wrote: > > o if there's a need to disable > just one of these, commit log needs to do a better job documenting the > usecase. The use case is simple. With this feature admins will be able to do what they were forced to do from Windows driver level but now at the bus level. Hence, (a) They need not have access to the guest VM to change or update windows driver registry settings. They can enable the same setting from admin management console without any access to VM. (b) It is more robust. No need to mess with driver settings. Incorrect settings can brick guest OS. Also no guest specific knowhow required. (c) It is more scalable since a single cluster wide setting can be used for all VM power ons and the management plane can take care of the rest automatically. No need to access individual VMs to enforce this. (d) I am told that the driver level solution does not persist across a reboot. Ani >>> >>> Looks like disabling both plug and unplug would also address these needs. >> >> No the driver level solution does not prevent hot plugging of devices but >> blocks just hot unplugging. The solution I am proposing tries to do the same >> but from the bus/hypervisor level. > > Why does the driver level solution need to prevent just hot unplugging? Because it not fair to prevent end users from hot plugging new devices when it is the (accidental?) hot unplugging of existing devices which causes issues. > > >> >>> -- >>> MST
Re: [PATCH v2 3/3] configure: add libdaxctl support
On 4/29/2020 12:23 AM, Joao Martins wrote:
On 4/15/20 4:35 AM, Jingqi Liu wrote:
Add a pair of configure options --{enable,disable}-libdaxctl to control
whether QEMU is compiled with libdaxctl [1]. Libdaxctl is a utility
library for managing the device dax subsystem.
QEMU uses mmap(2) to maps vNVDIMM backends and aligns the mapping
address to the page size (getpagesize(2)) by default. However, some
types of backends may require an alignment different than the page
size. The 'align' option is provided to memory-backend-file to allow
users to specify the proper alignment.
For device dax (e.g., /dev/dax0.0), the 'align' option needs to match
the alignment requirement of the device dax, which can be fetched
through the libdaxctl APIs.
[1] Libdaxctl is a part of ndctl project.
The project's repository is: https://github.com/pmem/ndctl
For more information about libdaxctl APIs, you can refer to the
comments in source code of: pmem/ndctl/daxctl/lib/libdaxctl.c.
Signed-off-by: Jingqi Liu
---
configure | 30 ++
1 file changed, 30 insertions(+)
diff --git a/configure b/configure
index e225a1e3ff..df1752cf08 100755
--- a/configure
+++ b/configure
@@ -509,6 +509,7 @@ libpmem=""
default_devices="yes"
plugins="no"
fuzzing="no"
+libdaxctl=""
supported_cpu="no"
supported_os="no"
@@ -1601,6 +1602,10 @@ for opt do
;;
--gdb=*) gdb_bin="$optarg"
;;
+ --enable-libdaxctl) libdaxctl=yes
+ ;;
+ --disable-libdaxctl) libdaxctl=no
+ ;;
*)
echo "ERROR: unknown option $opt"
echo "Try '$0 --help' for more information"
@@ -1894,6 +1899,7 @@ disabled with --disable-FEATURE, default is enabled if
available:
debug-mutex mutex debugging support
libpmem libpmem support
xkbcommon xkbcommon support
+ libdaxctl libdaxctl support
NOTE: The object files are built at the place where configure is launched
EOF
@@ -6190,6 +6196,25 @@ if test "$libpmem" != "no"; then
fi
fi
+##
+# check for libdaxctl
+
+if test "$libdaxctl" != "no"; then
+ if $pkg_config --exists "libdaxctl"; then
+ libdaxctl="yes"
+ libdaxctl_libs=$($pkg_config --libs libdaxctl)
+ libdaxctl_cflags=$($pkg_config --cflags libdaxctl)
+ libs_softmmu="$libs_softmmu $libdaxctl_libs"
+ QEMU_CFLAGS="$QEMU_CFLAGS $libdaxctl_cflags"
+ else
+ if test "$libdaxctl" = "yes" ; then
+ feature_not_found "libdaxctl" "Install libdaxctl"
+ fi
Region iteration APIs, align and path getter routines are only available since
libdaxctl v56/v57 (the latest is v68).
Not sure how likely this happens in today's distros but if we care about systems
with < v57 we should probably check that
daxctl_region_foreach/daxctl_region_get_align/daxctl_region_get_path symbols
exist? Or alternatively requiring v57 or up which serves as a bandage, but more
long term ... any usage of newer daxctl APIs will require the former.
Thanks for your suggestion.
Better to require v57 or up.
How about adding the following check:
+ if $pkg_config --atleast-version=57 "libdaxctl"; then
Jingqi
Re: [PATCH 16/17] spapr_pci: Drop some dead error handling
On Tue, 28 Apr 2020 18:34:18 +0200
Markus Armbruster wrote:
> chassis_from_bus() uses object_property_get_uint() to get property
> "chassis_nr" of the bridge device. Failure would be a programming
> error. Pass &error_abort, and simplify its callers.
>
> Cc: David Gibson
> Cc: qemu-...@nongnu.org
> Signed-off-by: Markus Armbruster
> ---
Reviewed-by: Greg Kurz
> hw/ppc/spapr_pci.c | 86 ++
> 1 file changed, 18 insertions(+), 68 deletions(-)
>
> diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
> index 1d73d05a0a..b6036be51c 100644
> --- a/hw/ppc/spapr_pci.c
> +++ b/hw/ppc/spapr_pci.c
> @@ -1203,46 +1203,36 @@ static SpaprDrc *drc_from_devfn(SpaprPhbState *phb,
> drc_id_from_devfn(phb, chassis, devfn));
> }
>
> -static uint8_t chassis_from_bus(PCIBus *bus, Error **errp)
> +static uint8_t chassis_from_bus(PCIBus *bus)
> {
> if (pci_bus_is_root(bus)) {
> return 0;
> } else {
> PCIDevice *bridge = pci_bridge_get_device(bus);
>
> -return object_property_get_uint(OBJECT(bridge), "chassis_nr", errp);
> +return object_property_get_uint(OBJECT(bridge), "chassis_nr",
> +&error_abort);
> }
> }
>
> static SpaprDrc *drc_from_dev(SpaprPhbState *phb, PCIDevice *dev)
> {
> -Error *local_err = NULL;
> -uint8_t chassis = chassis_from_bus(pci_get_bus(dev), &local_err);
> -
> -if (local_err) {
> -error_report_err(local_err);
> -return NULL;
> -}
> +uint8_t chassis = chassis_from_bus(pci_get_bus(dev));
>
> return drc_from_devfn(phb, chassis, dev->devfn);
> }
>
> -static void add_drcs(SpaprPhbState *phb, PCIBus *bus, Error **errp)
> +static void add_drcs(SpaprPhbState *phb, PCIBus *bus)
> {
> Object *owner;
> int i;
> uint8_t chassis;
> -Error *local_err = NULL;
>
> if (!phb->dr_enabled) {
> return;
> }
>
> -chassis = chassis_from_bus(bus, &local_err);
> -if (local_err) {
> -error_propagate(errp, local_err);
> -return;
> -}
> +chassis = chassis_from_bus(bus);
>
> if (pci_bus_is_root(bus)) {
> owner = OBJECT(phb);
> @@ -1256,21 +1246,16 @@ static void add_drcs(SpaprPhbState *phb, PCIBus *bus,
> Error **errp)
> }
> }
>
> -static void remove_drcs(SpaprPhbState *phb, PCIBus *bus, Error **errp)
> +static void remove_drcs(SpaprPhbState *phb, PCIBus *bus)
> {
> int i;
> uint8_t chassis;
> -Error *local_err = NULL;
>
> if (!phb->dr_enabled) {
> return;
> }
>
> -chassis = chassis_from_bus(bus, &local_err);
> -if (local_err) {
> -error_propagate(errp, local_err);
> -return;
> -}
> +chassis = chassis_from_bus(bus);
>
> for (i = PCI_SLOT_MAX * PCI_FUNC_MAX - 1; i >= 0; i--) {
> SpaprDrc *drc = drc_from_devfn(phb, chassis, i);
> @@ -1488,17 +1473,11 @@ int spapr_pci_dt_populate(SpaprDrc *drc,
> SpaprMachineState *spapr,
> }
>
> static void spapr_pci_bridge_plug(SpaprPhbState *phb,
> - PCIBridge *bridge,
> - Error **errp)
> + PCIBridge *bridge)
> {
> -Error *local_err = NULL;
> PCIBus *bus = pci_bridge_get_sec_bus(bridge);
>
> -add_drcs(phb, bus, &local_err);
> -if (local_err) {
> -error_propagate(errp, local_err);
> -return;
> -}
> +add_drcs(phb, bus);
> }
>
> static void spapr_pci_plug(HotplugHandler *plug_handler,
> @@ -1529,11 +1508,7 @@ static void spapr_pci_plug(HotplugHandler
> *plug_handler,
> g_assert(drc);
>
> if (pc->is_bridge) {
> -spapr_pci_bridge_plug(phb, PCI_BRIDGE(plugged_dev), &local_err);
> -if (local_err) {
> -error_propagate(errp, local_err);
> -return;
> -}
> +spapr_pci_bridge_plug(phb, PCI_BRIDGE(plugged_dev));
> }
>
> /* Following the QEMU convention used for PCIe multifunction
> @@ -1560,12 +1535,7 @@ static void spapr_pci_plug(HotplugHandler
> *plug_handler,
> spapr_drc_reset(drc);
> } else if (PCI_FUNC(pdev->devfn) == 0) {
> int i;
> -uint8_t chassis = chassis_from_bus(pci_get_bus(pdev), &local_err);
> -
> -if (local_err) {
> -error_propagate(errp, local_err);
> -return;
> -}
> +uint8_t chassis = chassis_from_bus(pci_get_bus(pdev));
>
> for (i = 0; i < 8; i++) {
> SpaprDrc *func_drc;
> @@ -1587,17 +1557,11 @@ out:
> }
>
> static void spapr_pci_bridge_unplug(SpaprPhbState *phb,
> -PCIBridge *bridge,
> -Error **errp)
> +PCIBridge *bridge)
> {
> -Error *local_err = NULL;
> PCIBus *bus = pci_bridge_get_sec_bus(bridge);
>
> -remove_drcs(phb, bus, &local_
Re: [PATCH 07/11] mips/malta: Fix create_cps() error handling
+Peter for crediting his advice.
On 4/29/20 7:59 AM, Markus Armbruster wrote:
Philippe Mathieu-Daudé writes:
On 4/24/20 9:20 PM, Markus Armbruster wrote:
The Error ** argument must be NULL, &error_abort, &error_fatal, or a
pointer to a variable containing NULL. Passing an argument of the
latter kind twice without clearing it in between is wrong: if the
first call sets an error, it no longer points to NULL for the second
create_cps() is wrong that way. The last calls treats an error as
fatal. Do that for the prior ones, too.
Fixes: bff384a4fbd5d0e86939092e74e766ef0f5f592c
Cc: Aleksandar Markovic
Cc: "Philippe Mathieu-Daudé"
Cc: Aurelien Jarno
Signed-off-by: Markus Armbruster
---
hw/mips/mips_malta.c | 15 ++-
1 file changed, 6 insertions(+), 9 deletions(-)
diff --git a/hw/mips/mips_malta.c b/hw/mips/mips_malta.c
index e4c4de1b4e..17bf41616b 100644
--- a/hw/mips/mips_malta.c
+++ b/hw/mips/mips_malta.c
@@ -1185,17 +1185,14 @@ static void create_cpu_without_cps(MachineState *ms,
static void create_cps(MachineState *ms, MaltaState *s,
qemu_irq *cbus_irq, qemu_irq *i8259_irq)
{
-Error *err = NULL;
-
sysbus_init_child_obj(OBJECT(s), "cps", OBJECT(&s->cps), sizeof(s->cps),
TYPE_MIPS_CPS);
-object_property_set_str(OBJECT(&s->cps), ms->cpu_type, "cpu-type", &err);
-object_property_set_int(OBJECT(&s->cps), ms->smp.cpus, "num-vp", &err);
-object_property_set_bool(OBJECT(&s->cps), true, "realized", &err);
-if (err != NULL) {
-error_report("%s", error_get_pretty(err));
In https://www.mail-archive.com/qemu-devel@nongnu.org/msg695645.html I
also remove "qemu/error-report.h" which is not needed once you remove
this call.
Missed it, sorry. I only reviewed the Coccinelle scripts [PATCH 1+3/7].
My bad for not Cc'ing you on the whole series, which is Error related,
and use the default get_maintainer.pl selection.
I'd replace my patch by yours to give you proper credit, but your commit
message mentions "the coccinelle script", presumably the one from PATCH
1/7, and that patch isn't quite ready in my opinion.
I'm not worried about credit, but duplicating effort or wasting time.
Peter once warned the problem with Coccinelle scripts is finding the
correct balance between time spent to improve QEMU codebase, and time
spent learning Coccinelle and improving a script that is often used only
once in a lifetime.
If the script is not provided, we ask for the script. If the script is
embedded in various patch descriptions, we ask to add it independently
for reuse or as example. Then the script must be almost perfect.
Meanwhile all the following patches referencing it, while reviewed, are
stuck.
Anyway back to your patch:
Reviewed-by: Philippe Mathieu-Daudé
Tested-by: Philippe Mathieu-Daudé
-exit(1);
-}
+object_property_set_str(OBJECT(&s->cps), ms->cpu_type, "cpu-type",
+&error_fatal);
+object_property_set_int(OBJECT(&s->cps), ms->smp.cpus, "num-vp",
+&error_fatal);
+object_property_set_bool(OBJECT(&s->cps), true, "realized",
+ &error_fatal);
sysbus_mmio_map_overlap(SYS_BUS_DEVICE(&s->cps), 0, 0, 1);
Re: [PATCH v2 for-5.1 0/9] qemu-option: Fix corner cases and clean up
Queued.
Re: [PATCH v2 00/14] Miscellaneous error handling fixes
Queued.
Re: [PATCH v2 0/4] smbus: SPD fixes
Queued.
Re: [PATCH 0/3] fuzz: Probably there is a better way to do this
Markus Armbruster writes: > ... the comment next to qos_set_machines_devices_available() wonders. > Yup, there is! Queued.
[PULL 10/32] qemu-img: Reject broken -o ""
qemu-img create, convert, amend, and measure use accumulate_options()
to merge multiple -o options. This is broken for -o "":
$ qemu-img create -f qcow2 -o backing_file=a -o "" -o
backing_fmt=raw,size=1M new.qcow2
qemu-img: warning: Could not verify backing image. This may become an error
in future versions.
Could not open 'a,backing_fmt=raw': No such file or directory
Formatting 'new.qcow2', fmt=qcow2 size=1048576
backing_file=a,,backing_fmt=raw cluster_size=65536 lazy_refcounts=off
refcount_bits=16
$ qemu-img info new.qcow2
image: new.qcow2
file format: qcow2
virtual size: 1 MiB (1048576 bytes)
disk size: 196 KiB
cluster_size: 65536
--> backing file: a,backing_fmt=raw
Format specific information:
compat: 1.1
lazy refcounts: false
refcount bits: 16
corrupt: false
Merging these three -o the obvious way is wrong, because it results in
an unwanted ',' escape:
backing_file=a,,backing_fmt=raw,size=1M
~~
We could silently drop -o "", but Kevin asked me to reject it instead.
Signed-off-by: Markus Armbruster
Message-Id: <20200415074927.19897-10-arm...@redhat.com>
Reviewed-by: Eric Blake
---
qemu-img.c | 6 --
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/qemu-img.c b/qemu-img.c
index cc51db7ed4..a2369766f0 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -229,14 +229,16 @@ static bool qemu_img_object_print_help(const char *type,
QemuOpts *opts)
* To make that work, @optarg must not start with ',' (or else a
* separating ',' preceding it gets escaped), and it must not end with
* an odd number of ',' (or else a separating ',' following it gets
- * escaped).
+ * escaped), or be empty (or else a separating ',' preceding it can
+ * escape a separating ',' following it).
+ *
*/
static bool is_valid_option_list(const char *optarg)
{
size_t len = strlen(optarg);
size_t i;
-if (optarg[0] == ',') {
+if (!optarg[0] || optarg[0] == ',') {
return false;
}
--
2.21.1
[PULL 03/32] qemu-options: Factor out get_opt_name_value() helper
The next commits will put it to use.
Signed-off-by: Markus Armbruster
Reviewed-by: Eric Blake
Reviewed-by: Kevin Wolf
Message-Id: <20200415074927.19897-3-arm...@redhat.com>
---
util/qemu-option.c | 102 +
1 file changed, 56 insertions(+), 46 deletions(-)
diff --git a/util/qemu-option.c b/util/qemu-option.c
index 97172b5eaa..f08f4bc458 100644
--- a/util/qemu-option.c
+++ b/util/qemu-option.c
@@ -805,61 +805,71 @@ void qemu_opts_print(QemuOpts *opts, const char
*separator)
}
}
+static const char *get_opt_name_value(const char *params,
+ const char *firstname,
+ char **name, char **value)
+{
+const char *p, *pe, *pc;
+
+pe = strchr(params, '=');
+pc = strchr(params, ',');
+
+if (!pe || (pc && pc < pe)) {
+/* found "foo,more" */
+if (firstname) {
+/* implicitly named first option */
+*name = g_strdup(firstname);
+p = get_opt_value(params, value);
+} else {
+/* option without value, must be a flag */
+p = get_opt_name(params, name, ',');
+if (strncmp(*name, "no", 2) == 0) {
+memmove(*name, *name + 2, strlen(*name + 2) + 1);
+*value = g_strdup("off");
+} else {
+*value = g_strdup("on");
+}
+}
+} else {
+/* found "foo=bar,more" */
+p = get_opt_name(params, name, '=');
+assert(*p == '=');
+p++;
+p = get_opt_value(p, value);
+}
+
+assert(!*p || *p == ',');
+if (*p == ',') {
+p++;
+}
+return p;
+}
+
static void opts_do_parse(QemuOpts *opts, const char *params,
const char *firstname, bool prepend,
bool *invalidp, Error **errp)
{
-char *option = NULL;
-char *value = NULL;
-const char *p,*pe,*pc;
Error *local_err = NULL;
+char *option, *value;
+const char *p;
-for (p = params; *p != '\0'; p++) {
-pe = strchr(p, '=');
-pc = strchr(p, ',');
-if (!pe || (pc && pc < pe)) {
-/* found "foo,more" */
-if (p == params && firstname) {
-/* implicitly named first option */
-option = g_strdup(firstname);
-p = get_opt_value(p, &value);
-} else {
-/* option without value, probably a flag */
-p = get_opt_name(p, &option, ',');
-if (strncmp(option, "no", 2) == 0) {
-memmove(option, option+2, strlen(option+2)+1);
-value = g_strdup("off");
-} else {
-value = g_strdup("on");
-}
-}
-} else {
-/* found "foo=bar,more" */
-p = get_opt_name(p, &option, '=');
-assert(*p == '=');
-p++;
-p = get_opt_value(p, &value);
-}
-if (strcmp(option, "id") != 0) {
-/* store and parse */
-opt_set(opts, option, value, prepend, invalidp, &local_err);
-value = NULL;
-if (local_err) {
-error_propagate(errp, local_err);
-goto cleanup;
-}
-}
-if (*p != ',') {
-break;
+for (p = params; *p;) {
+p = get_opt_name_value(p, firstname, &option, &value);
+firstname = NULL;
+
+if (!strcmp(option, "id")) {
+g_free(option);
+g_free(value);
+continue;
}
+
+opt_set(opts, option, value, prepend, invalidp, &local_err);
g_free(option);
-g_free(value);
-option = value = NULL;
+if (local_err) {
+error_propagate(errp, local_err);
+return;
+}
}
-
- cleanup:
-g_free(option);
-g_free(value);
}
/**
--
2.21.1
[PULL 00/32] Miscellaneous patches for 2020-04-29
The following changes since commit fdd76fecdde1ad444ff4deb7f1c4f7e4a1ef97d6:
Update version for v5.0.0 release (2020-04-28 17:46:57 +0100)
are available in the Git repository at:
git://repo.or.cz/qemu/armbru.git tags/pull-misc-2020-04-29
for you to fetch changes up to 8ef3a4be27efccd791d05e74b7b17d918f511a76:
qemu-option: pass NULL rather than 0 to the id of qemu_opts_set() (2020-04-29
08:01:52 +0200)
Miscellaneous patches for 2020-04-29
* Fix CLI option parsing corner cases
* Fix bugs on (unlikely) error paths
* Fix undefined behavior for silly option arguments
* Tidy up bamboo and sam460ex reporting of funny memory sizes
* Clean up error API violations
* A bit of refactoring here and there
Markus Armbruster (30):
tests-qemu-opts: Cover has_help_option(), qemu_opt_has_help_opt()
qemu-options: Factor out get_opt_name_value() helper
qemu-option: Fix sloppy recognition of "id=..." after ",,"
qemu-option: Fix has_help_option()'s sloppy parsing
test-qemu-opts: Simplify test_has_help_option() after bug fix
qemu-option: Avoid has_help_option() in qemu_opts_parse_noisily()
qemu-img: Factor out accumulate_options() helper
qemu-img: Move is_valid_option_list() to qemu-img.c and rewrite
qemu-img: Reject broken -o ""
cryptodev: Fix cryptodev_builtin_cleanup() error API violation
block/file-posix: Fix check_cache_dropped() error handling
cpus: Fix configure_icount() error API violation
cpus: Proper range-checking for -icount shift=N
arm/virt: Fix virt_machine_device_plug_cb() error API violation
fdc: Fix fallback=auto error handling
bochs-display: Fix vgamem=SIZE error handling
virtio-net: Fix duplex=... and speed=... error handling
xen/pt: Fix flawed conversion to realize()
io: Fix qio_channel_socket_close() error handling
migration/colo: Fix qmp_xen_colo_do_checkpoint() error handling
tests/test-logging: Fix test for -dfilter 0..0x
qga: Fix qmp_guest_get_memory_blocks() error handling
qga: Fix qmp_guest_suspend_{disk, ram}() error handling
sam460ex: Suppress useless warning on -m 32 and -m 64
smbus: Fix spd_data_generate() error API violation
bamboo, sam460ex: Tidy up error message for unsupported RAM size
smbus: Fix spd_data_generate() for number of banks > 2
Makefile: Drop unused, broken target recurse-fuzz
fuzz: Simplify how we compute available machines and types
libqos: Give get_machine_allocator() internal linkage
Masahiro Yamada (1):
qemu-option: pass NULL rather than 0 to the id of qemu_opts_set()
Philippe Mathieu-Daudé (1):
various: Remove suspicious '\' character outside of #define in C code
Makefile | 1 -
include/hw/i2c/smbus_eeprom.h | 2 +-
include/qemu/option.h | 1 -
tests/qtest/libqos/qos_external.h | 10 +-
backends/cryptodev-builtin.c | 10 +-
block/file-posix.c| 5 +-
block/replication.c | 4 +-
block/vhdx.c | 8 +-
cpus.c| 52 ++
dump/dump.c | 2 +-
hw/arm/virt.c | 4 +-
hw/block/fdc.c| 1 +
hw/display/bochs-display.c| 6 +-
hw/i2c/smbus_eeprom.c | 32 +-
hw/mips/mips_fulong2e.c | 10 +-
hw/net/virtio-net.c | 7 +-
hw/ppc/ppc4xx_devs.c | 8 +-
hw/ppc/sam460ex.c | 13 +--
hw/riscv/sifive_u.c | 2 +-
hw/scsi/scsi-disk.c | 2 +-
hw/sd/sdhci.c | 2 +-
hw/xen/xen_pt.c | 12 +--
io/channel-socket.c | 5 +-
migration/colo.c | 8 +-
qemu-img.c| 87 +---
qga/commands-posix.c | 3 +
qga/commands-win32.c | 14 +++
softmmu/vl.c | 10 +-
target/i386/cpu.c | 18 ++--
target/microblaze/cpu.c | 14 +--
target/ppc/translate_init.inc.c | 4 +-
tests/qtest/fuzz/qos_fuzz.c | 34 ++
tests/qtest/libqos/qos_external.c | 72 +
tests/qtest/qos-test.c| 29 --
tests/test-logging.c | 4 +-
tests/test-qemu-opts.c| 46 -
util/qemu-option.c| 210 +++---
37 files changed, 391 insertions(+), 361 deletions(-)
--
2.21.1
[PULL 07/32] qemu-option: Avoid has_help_option() in qemu_opts_parse_noisily()
When opts_parse() sets @invalidp to true, qemu_opts_parse_noisily()
uses has_help_option() to decide whether to print help. This parses
the input string a second time.
Easy to avoid: replace @invalidp by @help_wanted.
Signed-off-by: Markus Armbruster
Reviewed-by: Eric Blake
Message-Id: <20200415074927.19897-7-arm...@redhat.com>
---
util/qemu-option.c | 20 ++--
1 file changed, 10 insertions(+), 10 deletions(-)
diff --git a/util/qemu-option.c b/util/qemu-option.c
index 0abf26b61f..2d0d24ee27 100644
--- a/util/qemu-option.c
+++ b/util/qemu-option.c
@@ -519,7 +519,7 @@ int qemu_opt_unset(QemuOpts *opts, const char *name)
}
static void opt_set(QemuOpts *opts, const char *name, char *value,
-bool prepend, bool *invalidp, Error **errp)
+bool prepend, bool *help_wanted, Error **errp)
{
QemuOpt *opt;
const QemuOptDesc *desc;
@@ -529,8 +529,8 @@ static void opt_set(QemuOpts *opts, const char *name, char
*value,
if (!desc && !opts_accepts_any(opts)) {
g_free(value);
error_setg(errp, QERR_INVALID_PARAMETER, name);
-if (invalidp) {
-*invalidp = true;
+if (help_wanted && is_help_option(name)) {
+*help_wanted = true;
}
return;
}
@@ -827,7 +827,7 @@ static const char *get_opt_name_value(const char *params,
static void opts_do_parse(QemuOpts *opts, const char *params,
const char *firstname, bool prepend,
- bool *invalidp, Error **errp)
+ bool *help_wanted, Error **errp)
{
Error *local_err = NULL;
char *option, *value;
@@ -843,7 +843,7 @@ static void opts_do_parse(QemuOpts *opts, const char
*params,
continue;
}
-opt_set(opts, option, value, prepend, invalidp, &local_err);
+opt_set(opts, option, value, prepend, help_wanted, &local_err);
g_free(option);
if (local_err) {
error_propagate(errp, local_err);
@@ -903,7 +903,7 @@ void qemu_opts_do_parse(QemuOpts *opts, const char *params,
static QemuOpts *opts_parse(QemuOptsList *list, const char *params,
bool permit_abbrev, bool defaults,
-bool *invalidp, Error **errp)
+bool *help_wanted, Error **errp)
{
const char *firstname;
char *id = opts_parse_id(params);
@@ -928,7 +928,7 @@ static QemuOpts *opts_parse(QemuOptsList *list, const char
*params,
return NULL;
}
-opts_do_parse(opts, params, firstname, defaults, invalidp, &local_err);
+opts_do_parse(opts, params, firstname, defaults, help_wanted, &local_err);
if (local_err) {
error_propagate(errp, local_err);
qemu_opts_del(opts);
@@ -964,11 +964,11 @@ QemuOpts *qemu_opts_parse_noisily(QemuOptsList *list,
const char *params,
{
Error *err = NULL;
QemuOpts *opts;
-bool invalidp = false;
+bool help_wanted = false;
-opts = opts_parse(list, params, permit_abbrev, false, &invalidp, &err);
+opts = opts_parse(list, params, permit_abbrev, false, &help_wanted, &err);
if (err) {
-if (invalidp && has_help_option(params)) {
+if (help_wanted) {
qemu_opts_print_help(list, true);
error_free(err);
} else {
--
2.21.1
[PULL 24/32] qga: Fix qmp_guest_suspend_{disk, ram}() error handling
The Error ** argument must be NULL, &error_abort, &error_fatal, or a
pointer to a variable containing NULL. Passing an argument of the
latter kind twice without clearing it in between is wrong: if the
first call sets an error, it no longer points to NULL for the second
qmp_guest_suspend_disk() and qmp_guest_suspend_ram() pass @local_err
first to check_suspend_mode(), then to acquire_privilege(), then to
execute_async(). Continuing after errors here can only end in tears.
For instance, we risk tripping error_setv()'s assertion.
Fixes: aa59637ea1c6a4c83430933f9c44c43e6c3f1b69
Fixes: f54603b6aa765514b2519e74114a2f417759d727
Cc: Michael Roth
Signed-off-by: Markus Armbruster
Message-Id: <20200422130719.28225-15-arm...@redhat.com>
Reviewed-by: Eric Blake
Reviewed-by: Philippe Mathieu-Daudé
---
qga/commands-win32.c | 14 ++
1 file changed, 14 insertions(+)
diff --git a/qga/commands-win32.c b/qga/commands-win32.c
index 9717a8d52d..5ba56327dd 100644
--- a/qga/commands-win32.c
+++ b/qga/commands-win32.c
@@ -1322,9 +1322,16 @@ void qmp_guest_suspend_disk(Error **errp)
*mode = GUEST_SUSPEND_MODE_DISK;
check_suspend_mode(*mode, &local_err);
+if (local_err) {
+goto out;
+}
acquire_privilege(SE_SHUTDOWN_NAME, &local_err);
+if (local_err) {
+goto out;
+}
execute_async(do_suspend, mode, &local_err);
+out:
if (local_err) {
error_propagate(errp, local_err);
g_free(mode);
@@ -1338,9 +1345,16 @@ void qmp_guest_suspend_ram(Error **errp)
*mode = GUEST_SUSPEND_MODE_RAM;
check_suspend_mode(*mode, &local_err);
+if (local_err) {
+goto out;
+}
acquire_privilege(SE_SHUTDOWN_NAME, &local_err);
+if (local_err) {
+goto out;
+}
execute_async(do_suspend, mode, &local_err);
+out:
if (local_err) {
error_propagate(errp, local_err);
g_free(mode);
--
2.21.1
[PULL 08/32] qemu-img: Factor out accumulate_options() helper
Signed-off-by: Markus Armbruster
Reviewed-by: Eric Blake
Reviewed-by: Kevin Wolf
Message-Id: <20200415074927.19897-8-arm...@redhat.com>
---
qemu-img.c | 59 +-
1 file changed, 23 insertions(+), 36 deletions(-)
diff --git a/qemu-img.c b/qemu-img.c
index 821cbf610e..d36b21b758 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -223,6 +223,25 @@ static bool qemu_img_object_print_help(const char *type,
QemuOpts *opts)
return true;
}
+static int accumulate_options(char **options, char *optarg)
+{
+char *new_options;
+
+if (!is_valid_option_list(optarg)) {
+error_report("Invalid option list: %s", optarg);
+return -1;
+}
+
+if (!*options) {
+*options = g_strdup(optarg);
+} else {
+new_options = g_strdup_printf("%s,%s", *options, optarg);
+g_free(*options);
+*options = new_options;
+}
+return 0;
+}
+
static QemuOptsList qemu_source_opts = {
.name = "source",
.implied_opt_name = "file",
@@ -482,17 +501,9 @@ static int img_create(int argc, char **argv)
fmt = optarg;
break;
case 'o':
-if (!is_valid_option_list(optarg)) {
-error_report("Invalid option list: %s", optarg);
+if (accumulate_options(&options, optarg) < 0) {
goto fail;
}
-if (!options) {
-options = g_strdup(optarg);
-} else {
-char *old_options = options;
-options = g_strdup_printf("%s,%s", options, optarg);
-g_free(old_options);
-}
break;
case 'q':
quiet = true;
@@ -2127,17 +2138,9 @@ static int img_convert(int argc, char **argv)
s.compressed = true;
break;
case 'o':
-if (!is_valid_option_list(optarg)) {
-error_report("Invalid option list: %s", optarg);
+if (accumulate_options(&options, optarg) < 0) {
goto fail_getopt;
}
-if (!options) {
-options = g_strdup(optarg);
-} else {
-char *old_options = options;
-options = g_strdup_printf("%s,%s", options, optarg);
-g_free(old_options);
-}
break;
case 'l':
if (strstart(optarg, SNAPSHOT_OPT_BASE, NULL)) {
@@ -3953,18 +3956,10 @@ static int img_amend(int argc, char **argv)
help();
break;
case 'o':
-if (!is_valid_option_list(optarg)) {
-error_report("Invalid option list: %s", optarg);
+if (accumulate_options(&options, optarg) < 0) {
ret = -1;
goto out_no_progress;
}
-if (!options) {
-options = g_strdup(optarg);
-} else {
-char *old_options = options;
-options = g_strdup_printf("%s,%s", options, optarg);
-g_free(old_options);
-}
break;
case 'f':
fmt = optarg;
@@ -4855,17 +4850,9 @@ static int img_measure(int argc, char **argv)
out_fmt = optarg;
break;
case 'o':
-if (!is_valid_option_list(optarg)) {
-error_report("Invalid option list: %s", optarg);
+if (accumulate_options(&options, optarg) < 0) {
goto out;
}
-if (!options) {
-options = g_strdup(optarg);
-} else {
-char *old_options = options;
-options = g_strdup_printf("%s,%s", options, optarg);
-g_free(old_options);
-}
break;
case 'l':
if (strstart(optarg, SNAPSHOT_OPT_BASE, NULL)) {
--
2.21.1
[PULL 04/32] qemu-option: Fix sloppy recognition of "id=..." after ", , "
Signed-off-by: Markus Armbruster
Reviewed-by: Eric Blake
Reviewed-by: Kevin Wolf
Message-Id: <20200415074927.19897-4-arm...@redhat.com>
---
tests/test-qemu-opts.c | 4 ++--
util/qemu-option.c | 27 +++
2 files changed, 21 insertions(+), 10 deletions(-)
diff --git a/tests/test-qemu-opts.c b/tests/test-qemu-opts.c
index 88a3e7bdf4..8ff97268d8 100644
--- a/tests/test-qemu-opts.c
+++ b/tests/test-qemu-opts.c
@@ -500,10 +500,10 @@ static void test_opts_parse(void)
g_assert(!opts);
/* TODO Cover .merge_lists = true */
-/* Buggy ID recognition */
+/* Buggy ID recognition (fixed) */
opts = qemu_opts_parse(&opts_list_03, "x=,,id=bar", false, &error_abort);
g_assert_cmpuint(opts_count(opts), ==, 1);
-g_assert_cmpstr(qemu_opts_id(opts), ==, "bar"); /* BUG */
+g_assert(!qemu_opts_id(opts));
g_assert_cmpstr(qemu_opt_get(opts, "x"), ==, ",id=bar");
/* Anti-social ID */
diff --git a/util/qemu-option.c b/util/qemu-option.c
index f08f4bc458..d2956082bd 100644
--- a/util/qemu-option.c
+++ b/util/qemu-option.c
@@ -872,6 +872,24 @@ static void opts_do_parse(QemuOpts *opts, const char
*params,
}
}
+static char *opts_parse_id(const char *params)
+{
+const char *p;
+char *name, *value;
+
+for (p = params; *p;) {
+p = get_opt_name_value(p, NULL, &name, &value);
+if (!strcmp(name, "id")) {
+g_free(name);
+return value;
+}
+g_free(name);
+g_free(value);
+}
+
+return NULL;
+}
+
/**
* Store options parsed from @params into @opts.
* If @firstname is non-null, the first key=value in @params may omit
@@ -889,20 +907,13 @@ static QemuOpts *opts_parse(QemuOptsList *list, const
char *params,
bool *invalidp, Error **errp)
{
const char *firstname;
-char *id = NULL;
-const char *p;
+char *id = opts_parse_id(params);
QemuOpts *opts;
Error *local_err = NULL;
assert(!permit_abbrev || list->implied_opt_name);
firstname = permit_abbrev ? list->implied_opt_name : NULL;
-if (strncmp(params, "id=", 3) == 0) {
-get_opt_value(params + 3, &id);
-} else if ((p = strstr(params, ",id=")) != NULL) {
-get_opt_value(p + 4, &id);
-}
-
/*
* This code doesn't work for defaults && !list->merge_lists: when
* params has no id=, and list has an element with !opts->id, it
--
2.21.1
[PULL 05/32] qemu-option: Fix has_help_option()'s sloppy parsing
has_help_option() uses its own parser. It's inconsistent with
qemu_opts_parse(), as demonstrated by test-qemu-opts case
/qemu-opts/has_help_option. Fix by reusing the common parser.
Signed-off-by: Markus Armbruster
Message-Id: <20200415074927.19897-5-arm...@redhat.com>
Reviewed-by: Eric Blake
---
tests/test-qemu-opts.c | 4 ++--
util/qemu-option.c | 39 +++
2 files changed, 21 insertions(+), 22 deletions(-)
diff --git a/tests/test-qemu-opts.c b/tests/test-qemu-opts.c
index 8ff97268d8..77c944c4aa 100644
--- a/tests/test-qemu-opts.c
+++ b/tests/test-qemu-opts.c
@@ -749,8 +749,8 @@ static void test_has_help_option(void)
{ "a=0,?,b", true, true, true },
{ "help,b=1", true, true, false },
{ "?,b=1", true, true, false },
-{ "a,b,,help", false /* BUG */, true, true },
-{ "a,b,,?", false /* BUG */, true, true },
+{ "a,b,,help", true, true, true },
+{ "a,b,,?", true, true, true },
};
int i;
QemuOpts *opts;
diff --git a/util/qemu-option.c b/util/qemu-option.c
index d2956082bd..0abf26b61f 100644
--- a/util/qemu-option.c
+++ b/util/qemu-option.c
@@ -165,26 +165,6 @@ void parse_option_size(const char *name, const char *value,
*ret = size;
}
-bool has_help_option(const char *param)
-{
-const char *p = param;
-bool result = false;
-
-while (*p && !result) {
-char *value;
-
-p = get_opt_value(p, &value);
-if (*p) {
-p++;
-}
-
-result = is_help_option(value);
-g_free(value);
-}
-
-return result;
-}
-
bool is_valid_option_list(const char *p)
{
char *value = NULL;
@@ -890,6 +870,25 @@ static char *opts_parse_id(const char *params)
return NULL;
}
+bool has_help_option(const char *params)
+{
+const char *p;
+char *name, *value;
+bool ret;
+
+for (p = params; *p;) {
+p = get_opt_name_value(p, NULL, &name, &value);
+ret = is_help_option(name);
+g_free(name);
+g_free(value);
+if (ret) {
+return true;
+}
+}
+
+return false;
+}
+
/**
* Store options parsed from @params into @opts.
* If @firstname is non-null, the first key=value in @params may omit
--
2.21.1
[PULL 25/32] sam460ex: Suppress useless warning on -m 32 and -m 64
Requesting 32 or 64 MiB of RAM with the sam460ex machine type produces
a useless warning:
qemu-system-ppc: warning: Memory size is too small for SDRAM type,
adjusting type
This is because sam460ex_init() asks spd_data_generate() for DDR2,
which is impossible, so spd_data_generate() corrects it to DDR.
The warning goes back to commit 08fd99179a "sam460ex: Clean up SPD
EEPROM creation".
Make sam460ex_init() pass the correct SDRAM type to get rid of the
warning.
Signed-off-by: Markus Armbruster
Message-Id: <20200422134815.1584-2-arm...@redhat.com>
Reviewed-by: Philippe Mathieu-Daudé
---
hw/ppc/sam460ex.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/hw/ppc/sam460ex.c b/hw/ppc/sam460ex.c
index 898453cf30..1e3eaac0db 100644
--- a/hw/ppc/sam460ex.c
+++ b/hw/ppc/sam460ex.c
@@ -335,7 +335,8 @@ static void sam460ex_init(MachineState *machine)
dev = sysbus_create_simple(TYPE_PPC4xx_I2C, 0x4ef600700, uic[0][2]);
i2c = PPC4xx_I2C(dev)->bus;
/* SPD EEPROM on RAM module */
-spd_data = spd_data_generate(DDR2, ram_sizes[0], &err);
+spd_data = spd_data_generate(ram_sizes[0] < 128 * MiB ? DDR : DDR2,
+ ram_sizes[0], &err);
if (err) {
warn_report_err(err);
}
--
2.21.1
[PULL 02/32] tests-qemu-opts: Cover has_help_option(), qemu_opt_has_help_opt()
The two turn out to be inconsistent for "a,b,,help". Test case
marked /* BUG */.
Signed-off-by: Markus Armbruster
Message-Id: <20200415074927.19897-2-arm...@redhat.com>
Reviewed-by: Eric Blake
---
tests/test-qemu-opts.c | 44 ++
1 file changed, 44 insertions(+)
diff --git a/tests/test-qemu-opts.c b/tests/test-qemu-opts.c
index ef96e84aed..88a3e7bdf4 100644
--- a/tests/test-qemu-opts.c
+++ b/tests/test-qemu-opts.c
@@ -728,6 +728,49 @@ static void test_opts_parse_size(void)
qemu_opts_reset(&opts_list_02);
}
+static void test_has_help_option(void)
+{
+static const struct {
+const char *params;
+/* expected value of has_help_option() */
+bool expect_has_help_option;
+/* expected value of qemu_opt_has_help_opt() with implied=false */
+bool expect_opt_has_help_opt;
+/* expected value of qemu_opt_has_help_opt() with implied=true */
+bool expect_opt_has_help_opt_implied;
+} test[] = {
+{ "help", true, true, false },
+{ "?", true, true, false },
+{ "helpme", false, false, false },
+{ "?me", false, false, false },
+{ "a,help", true, true, true },
+{ "a,?", true, true, true },
+{ "a=0,help,b", true, true, true },
+{ "a=0,?,b", true, true, true },
+{ "help,b=1", true, true, false },
+{ "?,b=1", true, true, false },
+{ "a,b,,help", false /* BUG */, true, true },
+{ "a,b,,?", false /* BUG */, true, true },
+};
+int i;
+QemuOpts *opts;
+
+for (i = 0; i < ARRAY_SIZE(test); i++) {
+g_assert_cmpint(has_help_option(test[i].params),
+==, test[i].expect_has_help_option);
+opts = qemu_opts_parse(&opts_list_03, test[i].params, false,
+ &error_abort);
+g_assert_cmpint(qemu_opt_has_help_opt(opts),
+==, test[i].expect_opt_has_help_opt);
+qemu_opts_del(opts);
+opts = qemu_opts_parse(&opts_list_03, test[i].params, true,
+ &error_abort);
+g_assert_cmpint(qemu_opt_has_help_opt(opts),
+==, test[i].expect_opt_has_help_opt_implied);
+qemu_opts_del(opts);
+}
+}
+
static void append_verify_list_01(QemuOptDesc *desc, bool with_overlapping)
{
int i = 0;
@@ -990,6 +1033,7 @@ int main(int argc, char *argv[])
g_test_add_func("/qemu-opts/opts_parse/bool", test_opts_parse_bool);
g_test_add_func("/qemu-opts/opts_parse/number", test_opts_parse_number);
g_test_add_func("/qemu-opts/opts_parse/size", test_opts_parse_size);
+g_test_add_func("/qemu-opts/has_help_option", test_has_help_option);
g_test_add_func("/qemu-opts/append_to_null", test_opts_append_to_null);
g_test_add_func("/qemu-opts/append", test_opts_append);
g_test_add_func("/qemu-opts/to_qdict/basic", test_opts_to_qdict_basic);
--
2.21.1
[PULL 06/32] test-qemu-opts: Simplify test_has_help_option() after bug fix
Signed-off-by: Markus Armbruster
Reviewed-by: Eric Blake
Reviewed-by: Kevin Wolf
Message-Id: <20200415074927.19897-6-arm...@redhat.com>
---
tests/test-qemu-opts.c | 36 +---
1 file changed, 17 insertions(+), 19 deletions(-)
diff --git a/tests/test-qemu-opts.c b/tests/test-qemu-opts.c
index 77c944c4aa..2a0f42a09b 100644
--- a/tests/test-qemu-opts.c
+++ b/tests/test-qemu-opts.c
@@ -732,41 +732,39 @@ static void test_has_help_option(void)
{
static const struct {
const char *params;
-/* expected value of has_help_option() */
-bool expect_has_help_option;
/* expected value of qemu_opt_has_help_opt() with implied=false */
-bool expect_opt_has_help_opt;
+bool expect;
/* expected value of qemu_opt_has_help_opt() with implied=true */
-bool expect_opt_has_help_opt_implied;
+bool expect_implied;
} test[] = {
-{ "help", true, true, false },
-{ "?", true, true, false },
-{ "helpme", false, false, false },
-{ "?me", false, false, false },
-{ "a,help", true, true, true },
-{ "a,?", true, true, true },
-{ "a=0,help,b", true, true, true },
-{ "a=0,?,b", true, true, true },
-{ "help,b=1", true, true, false },
-{ "?,b=1", true, true, false },
-{ "a,b,,help", true, true, true },
-{ "a,b,,?", true, true, true },
+{ "help", true, false },
+{ "?", true, false },
+{ "helpme", false, false },
+{ "?me", false, false },
+{ "a,help", true, true },
+{ "a,?", true, true },
+{ "a=0,help,b", true, true },
+{ "a=0,?,b", true, true },
+{ "help,b=1", true, false },
+{ "?,b=1", true, false },
+{ "a,b,,help", true, true },
+{ "a,b,,?", true, true },
};
int i;
QemuOpts *opts;
for (i = 0; i < ARRAY_SIZE(test); i++) {
g_assert_cmpint(has_help_option(test[i].params),
-==, test[i].expect_has_help_option);
+==, test[i].expect);
opts = qemu_opts_parse(&opts_list_03, test[i].params, false,
&error_abort);
g_assert_cmpint(qemu_opt_has_help_opt(opts),
-==, test[i].expect_opt_has_help_opt);
+==, test[i].expect);
qemu_opts_del(opts);
opts = qemu_opts_parse(&opts_list_03, test[i].params, true,
&error_abort);
g_assert_cmpint(qemu_opt_has_help_opt(opts),
-==, test[i].expect_opt_has_help_opt_implied);
+==, test[i].expect_implied);
qemu_opts_del(opts);
}
}
--
2.21.1
[PULL 31/32] libqos: Give get_machine_allocator() internal linkage
Signed-off-by: Markus Armbruster
Message-Id: <20200424071142.3525-4-arm...@redhat.com>
Reviewed-by: Philippe Mathieu-Daudé
---
tests/qtest/libqos/qos_external.h | 2 --
tests/qtest/libqos/qos_external.c | 2 +-
2 files changed, 1 insertion(+), 3 deletions(-)
diff --git a/tests/qtest/libqos/qos_external.h
b/tests/qtest/libqos/qos_external.h
index f63388cb30..72d7f91707 100644
--- a/tests/qtest/libqos/qos_external.h
+++ b/tests/qtest/libqos/qos_external.h
@@ -18,7 +18,6 @@
#ifndef QOS_EXTERNAL_H
#define QOS_EXTERNAL_H
-#include "libqos/qgraph.h"
#include "libqos/malloc.h"
#include "qapi/qapi-types-machine.h"
@@ -26,7 +25,6 @@
void machines_apply_to_node(MachineInfoList *mach_info);
void types_apply_to_node(ObjectTypeInfoList *type_info);
-QGuestAllocator *get_machine_allocator(QOSGraphObject *obj);
void *allocate_objects(QTestState *qts, char **path, QGuestAllocator
**p_alloc);
#endif
diff --git a/tests/qtest/libqos/qos_external.c
b/tests/qtest/libqos/qos_external.c
index c707dac3b9..9f5180e18d 100644
--- a/tests/qtest/libqos/qos_external.c
+++ b/tests/qtest/libqos/qos_external.c
@@ -66,7 +66,7 @@ void types_apply_to_node(ObjectTypeInfoList *type_info)
}
}
-QGuestAllocator *get_machine_allocator(QOSGraphObject *obj)
+static QGuestAllocator *get_machine_allocator(QOSGraphObject *obj)
{
return obj->get_driver(obj, "memory");
}
--
2.21.1
[PULL 27/32] bamboo, sam460ex: Tidy up error message for unsupported RAM size
Improve
$ ppc-softmmu/qemu-system-ppc -M sam460ex -m 4096
qemu-system-ppc: Max 1 banks of 2048 ,1024 ,512 ,256 ,128 ,64 ,32 MB
DIMM/bank supported
qemu-system-ppc: Possible valid RAM size: 2048
to
qemu-system-ppc: at most 1 bank of 2048, 1024, 512, 256, 128, 64, 32 MiB
each supported
Possible valid RAM size: 1024 MiB
Signed-off-by: Markus Armbruster
Message-Id: <20200422134815.1584-4-arm...@redhat.com>
Reviewed-by: BALATON Zoltan
Reviewed-by: Philippe Mathieu-Daudé
---
hw/ppc/ppc4xx_devs.c | 8
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/hw/ppc/ppc4xx_devs.c b/hw/ppc/ppc4xx_devs.c
index 3376c43ff5..f1651e04d9 100644
--- a/hw/ppc/ppc4xx_devs.c
+++ b/hw/ppc/ppc4xx_devs.c
@@ -716,11 +716,11 @@ void ppc4xx_sdram_banks(MemoryRegion *ram, int nr_banks,
for (i = 0; sdram_bank_sizes[i]; i++) {
g_string_append_printf(s, "%" PRIi64 "%s",
sdram_bank_sizes[i] / MiB,
- sdram_bank_sizes[i + 1] ? " ," : "");
+ sdram_bank_sizes[i + 1] ? ", " : "");
}
-error_report("Max %d banks of %s MB DIMM/bank supported",
-nr_banks, s->str);
-error_report("Possible valid RAM size: %" PRIi64,
+error_report("at most %d bank%s of %s MiB each supported",
+ nr_banks, nr_banks == 1 ? "" : "s", s->str);
+error_printf("Possible valid RAM size: %" PRIi64 " MiB \n",
used_size ? used_size / MiB : sdram_bank_sizes[i - 1] / MiB);
g_string_free(s, true);
--
2.21.1
[PULL 20/32] io: Fix qio_channel_socket_close() error handling
The Error ** argument must be NULL, &error_abort, &error_fatal, or a
pointer to a variable containing NULL. Passing an argument of the
latter kind twice without clearing it in between is wrong: if the
first call sets an error, it no longer points to NULL for the second
call.
qio_channel_socket_close() passes @errp first to
socket_listen_cleanup(), and then, if closesocket() fails, to
error_setg_errno(). If socket_listen_cleanup() failed, this will trip
the assertion in error_setv().
Fix by ignoring a second error.
Fixes: 73564c407caedf992a1c688b5fea776a8b56ba2a
Cc: Daniel P. Berrangé
Signed-off-by: Markus Armbruster
Reviewed-by: Daniel P. Berrangé
Message-Id: <20200422130719.28225-11-arm...@redhat.com>
---
io/channel-socket.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/io/channel-socket.c b/io/channel-socket.c
index b74f5b92a0..e1b4667087 100644
--- a/io/channel-socket.c
+++ b/io/channel-socket.c
@@ -704,6 +704,7 @@ qio_channel_socket_close(QIOChannel *ioc,
{
QIOChannelSocket *sioc = QIO_CHANNEL_SOCKET(ioc);
int rc = 0;
+Error *err = NULL;
if (sioc->fd != -1) {
#ifdef WIN32
@@ -715,8 +716,8 @@ qio_channel_socket_close(QIOChannel *ioc,
if (closesocket(sioc->fd) < 0) {
sioc->fd = -1;
-error_setg_errno(errp, errno,
- "Unable to close socket");
+error_setg_errno(&err, errno, "Unable to close socket");
+error_propagate(errp, err);
return -1;
}
sioc->fd = -1;
--
2.21.1
[PULL 16/32] fdc: Fix fallback=auto error handling
fdctrl_realize_common() rejects fallback=auto. Used by devices
"isa-fdc", "sysbus-fdc", "SUNW,fdtwo". The error handling is broken:
$ qemu-system-x86_64 -nodefaults -device isa-fdc,fallback=auto,driveA=fd0
-drive if=none,id=fd0
**
ERROR:/work/armbru/qemu/hw/block/fdc.c:434:pick_drive_type: assertion
failed: (drv->drive != FLOPPY_DRIVE_TYPE_AUTO)
Aborted (core dumped)
Cause: fdctrl_realize_common() neglects to bail out after setting the
error. Fix that.
Fixes: a73275dd6fc3bfda33165bebc28e0c33c20cb0a0
Cc: John Snow
Signed-off-by: Markus Armbruster
Reviewed-by: Philippe Mathieu-Daudé
Message-Id: <20200422130719.28225-7-arm...@redhat.com>
---
hw/block/fdc.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/hw/block/fdc.c b/hw/block/fdc.c
index 33bc9e2f92..9628cc171e 100644
--- a/hw/block/fdc.c
+++ b/hw/block/fdc.c
@@ -2615,6 +2615,7 @@ static void fdctrl_realize_common(DeviceState *dev,
FDCtrl *fdctrl,
if (fdctrl->fallback == FLOPPY_DRIVE_TYPE_AUTO) {
error_setg(errp, "Cannot choose a fallback FDrive type of 'auto'");
+return;
}
/* Fill 'command_to_handler' lookup table */
--
2.21.1
[PULL 09/32] qemu-img: Move is_valid_option_list() to qemu-img.c and rewrite
is_valid_option_list()'s purpose is ensuring qemu-img.c's can safely
join multiple parameter strings separated by ',' like this:
g_strdup_printf("%s,%s", params1, params2);
How it does that is anything but obvious. A close reading of the code
reveals that it fails exactly when its argument starts with ',' or
ends with an odd number of ','. Makes sense, actually, because when
the argument starts with ',', a separating ',' preceding it would get
escaped, and when it ends with an odd number of ',', a separating ','
following it would get escaped.
Move it to qemu-img.c and rewrite it the obvious way.
Signed-off-by: Markus Armbruster
Reviewed-by: Eric Blake
Message-Id: <20200415074927.19897-9-arm...@redhat.com>
---
include/qemu/option.h | 1 -
qemu-img.c| 26 ++
util/qemu-option.c| 22 --
3 files changed, 26 insertions(+), 23 deletions(-)
diff --git a/include/qemu/option.h b/include/qemu/option.h
index 844587cab3..eb4097889d 100644
--- a/include/qemu/option.h
+++ b/include/qemu/option.h
@@ -33,7 +33,6 @@ const char *get_opt_value(const char *p, char **value);
void parse_option_size(const char *name, const char *value,
uint64_t *ret, Error **errp);
bool has_help_option(const char *param);
-bool is_valid_option_list(const char *param);
enum QemuOptType {
QEMU_OPT_STRING = 0, /* no parsing (use string as-is)
*/
diff --git a/qemu-img.c b/qemu-img.c
index d36b21b758..cc51db7ed4 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -223,6 +223,32 @@ static bool qemu_img_object_print_help(const char *type,
QemuOpts *opts)
return true;
}
+/*
+ * Is @optarg safe for accumulate_options()?
+ * It is when multiple of them can be joined together separated by ','.
+ * To make that work, @optarg must not start with ',' (or else a
+ * separating ',' preceding it gets escaped), and it must not end with
+ * an odd number of ',' (or else a separating ',' following it gets
+ * escaped).
+ */
+static bool is_valid_option_list(const char *optarg)
+{
+size_t len = strlen(optarg);
+size_t i;
+
+if (optarg[0] == ',') {
+return false;
+}
+
+for (i = len; i > 0 && optarg[i - 1] == ','; i--) {
+}
+if ((len - i) % 2) {
+return false;
+}
+
+return true;
+}
+
static int accumulate_options(char **options, char *optarg)
{
char *new_options;
diff --git a/util/qemu-option.c b/util/qemu-option.c
index 2d0d24ee27..9542988183 100644
--- a/util/qemu-option.c
+++ b/util/qemu-option.c
@@ -165,28 +165,6 @@ void parse_option_size(const char *name, const char *value,
*ret = size;
}
-bool is_valid_option_list(const char *p)
-{
-char *value = NULL;
-bool result = false;
-
-while (*p) {
-p = get_opt_value(p, &value);
-if ((*p && !*++p) ||
-(!*value || *value == ',')) {
-goto out;
-}
-
-g_free(value);
-value = NULL;
-}
-
-result = true;
-out:
-g_free(value);
-return result;
-}
-
static const char *opt_type_to_string(enum QemuOptType type)
{
switch (type) {
--
2.21.1
[PULL 19/32] xen/pt: Fix flawed conversion to realize()
The conversion of xen_pt_initfn() to xen_pt_realize() blindly replaced
XEN_PT_ERR() by error_setg(). Several error conditions that did not
fail xen_pt_initfn() now fail xen_pt_realize(). Unsurprisingly, the
cleanup on these errors looks highly suspicious.
Revert the inappropriate replacements.
Fixes: 5a11d0f7549e24a10e178a9dc8ff5e698031d9a6
Cc: Stefano Stabellini
Cc: Anthony Perard
Cc: Paul Durrant
Cc: xen-de...@lists.xenproject.org
Signed-off-by: Markus Armbruster
Reviewed-by: Paul Durrant
Message-Id: <20200422130719.28225-10-arm...@redhat.com>
---
hw/xen/xen_pt.c | 12 ++--
1 file changed, 6 insertions(+), 6 deletions(-)
diff --git a/hw/xen/xen_pt.c b/hw/xen/xen_pt.c
index b91082cb8b..81d5ad8da7 100644
--- a/hw/xen/xen_pt.c
+++ b/hw/xen/xen_pt.c
@@ -858,8 +858,8 @@ static void xen_pt_realize(PCIDevice *d, Error **errp)
rc = xc_physdev_map_pirq(xen_xc, xen_domid, machine_irq, &pirq);
if (rc < 0) {
-error_setg_errno(errp, errno, "Mapping machine irq %u to"
- " pirq %i failed", machine_irq, pirq);
+XEN_PT_ERR(d, "Mapping machine irq %u to pirq %i failed, (err: %d)\n",
+ machine_irq, pirq, errno);
/* Disable PCI intx assertion (turn on bit10 of devctl) */
cmd |= PCI_COMMAND_INTX_DISABLE;
@@ -880,8 +880,8 @@ static void xen_pt_realize(PCIDevice *d, Error **errp)
PCI_SLOT(d->devfn),
e_intx);
if (rc < 0) {
-error_setg_errno(errp, errno, "Binding of interrupt %u failed",
- e_intx);
+XEN_PT_ERR(d, "Binding of interrupt %i failed! (err: %d)\n",
+ e_intx, errno);
/* Disable PCI intx assertion (turn on bit10 of devctl) */
cmd |= PCI_COMMAND_INTX_DISABLE;
@@ -889,8 +889,8 @@ static void xen_pt_realize(PCIDevice *d, Error **errp)
if (xen_pt_mapped_machine_irq[machine_irq] == 0) {
if (xc_physdev_unmap_pirq(xen_xc, xen_domid, machine_irq)) {
-error_setg_errno(errp, errno, "Unmapping of machine"
-" interrupt %u failed", machine_irq);
+XEN_PT_ERR(d, "Unmapping of machine interrupt %i failed!"
+ " (err: %d)\n", machine_irq, errno);
}
}
s->machine_irq = 0;
--
2.21.1
[PULL 32/32] qemu-option: pass NULL rather than 0 to the id of qemu_opts_set()
From: Masahiro Yamada
The second argument 'id' is a pointer. Pass NULL rather than 0.
Signed-off-by: Masahiro Yamada
Message-Id: <20200427005704.2475782-1-masahi...@kernel.org>
Reviewed-by: Markus Armbruster
Signed-off-by: Markus Armbruster
---
softmmu/vl.c | 10 +-
1 file changed, 5 insertions(+), 5 deletions(-)
diff --git a/softmmu/vl.c b/softmmu/vl.c
index 32c0047889..afd2615fb3 100644
--- a/softmmu/vl.c
+++ b/softmmu/vl.c
@@ -3059,19 +3059,19 @@ void qemu_init(int argc, char **argv, char **envp)
}
break;
case QEMU_OPTION_kernel:
-qemu_opts_set(qemu_find_opts("machine"), 0, "kernel", optarg,
+qemu_opts_set(qemu_find_opts("machine"), NULL, "kernel",
optarg,
&error_abort);
break;
case QEMU_OPTION_initrd:
-qemu_opts_set(qemu_find_opts("machine"), 0, "initrd", optarg,
+qemu_opts_set(qemu_find_opts("machine"), NULL, "initrd",
optarg,
&error_abort);
break;
case QEMU_OPTION_append:
-qemu_opts_set(qemu_find_opts("machine"), 0, "append", optarg,
+qemu_opts_set(qemu_find_opts("machine"), NULL, "append",
optarg,
&error_abort);
break;
case QEMU_OPTION_dtb:
-qemu_opts_set(qemu_find_opts("machine"), 0, "dtb", optarg,
+qemu_opts_set(qemu_find_opts("machine"), NULL, "dtb", optarg,
&error_abort);
break;
case QEMU_OPTION_cdrom:
@@ -3182,7 +3182,7 @@ void qemu_init(int argc, char **argv, char **envp)
}
break;
case QEMU_OPTION_bios:
-qemu_opts_set(qemu_find_opts("machine"), 0, "firmware", optarg,
+qemu_opts_set(qemu_find_opts("machine"), NULL, "firmware",
optarg,
&error_abort);
break;
case QEMU_OPTION_singlestep:
--
2.21.1
[PULL 01/32] various: Remove suspicious '\' character outside of #define in C code
From: Philippe Mathieu-Daudé
Fixes the following coccinelle warnings:
$ spatch --sp-file --verbose-parsing ... \
scripts/coccinelle/remove_local_err.cocci
...
SUSPICIOUS: a \ character appears outside of a #define at
./target/ppc/translate_init.inc.c:5213
SUSPICIOUS: a \ character appears outside of a #define at
./target/ppc/translate_init.inc.c:5261
SUSPICIOUS: a \ character appears outside of a #define at
./target/microblaze/cpu.c:166
SUSPICIOUS: a \ character appears outside of a #define at
./target/microblaze/cpu.c:167
SUSPICIOUS: a \ character appears outside of a #define at
./target/microblaze/cpu.c:169
SUSPICIOUS: a \ character appears outside of a #define at
./target/microblaze/cpu.c:170
SUSPICIOUS: a \ character appears outside of a #define at
./target/microblaze/cpu.c:171
SUSPICIOUS: a \ character appears outside of a #define at
./target/microblaze/cpu.c:172
SUSPICIOUS: a \ character appears outside of a #define at
./target/microblaze/cpu.c:173
SUSPICIOUS: a \ character appears outside of a #define at
./target/i386/cpu.c:5787
SUSPICIOUS: a \ character appears outside of a #define at
./target/i386/cpu.c:5789
SUSPICIOUS: a \ character appears outside of a #define at
./target/i386/cpu.c:5800
SUSPICIOUS: a \ character appears outside of a #define at
./target/i386/cpu.c:5801
SUSPICIOUS: a \ character appears outside of a #define at
./target/i386/cpu.c:5802
SUSPICIOUS: a \ character appears outside of a #define at
./target/i386/cpu.c:5804
SUSPICIOUS: a \ character appears outside of a #define at
./target/i386/cpu.c:5805
SUSPICIOUS: a \ character appears outside of a #define at
./target/i386/cpu.c:5806
SUSPICIOUS: a \ character appears outside of a #define at
./target/i386/cpu.c:6329
SUSPICIOUS: a \ character appears outside of a #define at ./hw/sd/sdhci.c:1133
SUSPICIOUS: a \ character appears outside of a #define at
./hw/scsi/scsi-disk.c:3081
SUSPICIOUS: a \ character appears outside of a #define at
./hw/net/virtio-net.c:1529
SUSPICIOUS: a \ character appears outside of a #define at
./hw/riscv/sifive_u.c:468
SUSPICIOUS: a \ character appears outside of a #define at ./dump/dump.c:1895
SUSPICIOUS: a \ character appears outside of a #define at ./block/vhdx.c:2209
SUSPICIOUS: a \ character appears outside of a #define at ./block/vhdx.c:2215
SUSPICIOUS: a \ character appears outside of a #define at ./block/vhdx.c:2221
SUSPICIOUS: a \ character appears outside of a #define at ./block/vhdx.c:
SUSPICIOUS: a \ character appears outside of a #define at
./block/replication.c:172
SUSPICIOUS: a \ character appears outside of a #define at
./block/replication.c:173
Reviewed-by: Marc-André Lureau
Signed-off-by: Philippe Mathieu-Daudé
Message-Id: <20200412223619.11284-2-f4...@amsat.org>
Reviewed-by: Alistair Francis
Acked-by: David Gibson
Signed-off-by: Markus Armbruster
---
block/replication.c | 4 ++--
block/vhdx.c| 8
dump/dump.c | 2 +-
hw/net/virtio-net.c | 2 +-
hw/riscv/sifive_u.c | 2 +-
hw/scsi/scsi-disk.c | 2 +-
hw/sd/sdhci.c | 2 +-
target/i386/cpu.c | 18 +-
target/microblaze/cpu.c | 14 +++---
target/ppc/translate_init.inc.c | 4 ++--
10 files changed, 29 insertions(+), 29 deletions(-)
diff --git a/block/replication.c b/block/replication.c
index da013c2041..971f0fe266 100644
--- a/block/replication.c
+++ b/block/replication.c
@@ -172,8 +172,8 @@ static void replication_child_perm(BlockDriverState *bs,
BdrvChild *c,
if ((bs->open_flags & (BDRV_O_INACTIVE | BDRV_O_RDWR)) == BDRV_O_RDWR) {
*nperm |= BLK_PERM_WRITE;
}
-*nshared = BLK_PERM_CONSISTENT_READ \
- | BLK_PERM_WRITE \
+*nshared = BLK_PERM_CONSISTENT_READ
+ | BLK_PERM_WRITE
| BLK_PERM_WRITE_UNCHANGED;
return;
}
diff --git a/block/vhdx.c b/block/vhdx.c
index 33e57cd656..e16fdc2f2d 100644
--- a/block/vhdx.c
+++ b/block/vhdx.c
@@ -2206,20 +2206,20 @@ static QemuOptsList vhdx_create_opts = {
.name = VHDX_BLOCK_OPT_BLOCK_SIZE,
.type = QEMU_OPT_SIZE,
.def_value_str = stringify(0),
- .help = "Block Size; min 1MB, max 256MB. " \
+ .help = "Block Size; min 1MB, max 256MB. "
"0 means auto-calculate based on image size."
},
{
.name = BLOCK_OPT_SUBFMT,
.type = QEMU_OPT_STRING,
- .help = "VHDX format type, can be either 'dynamic' or 'fixed'. "\
+ .help = "VHDX format type, can be either 'dynamic' or 'fixed'. "
"Default is 'dynamic'."
},
{
.name = VHDX_BLOCK_OPT_ZERO,
.type = QEMU_OPT_BOOL,
- .help = "Force use of payload blocks of type 'ZERO'. "\
- "Non-standard, but default. Do not set to 'off'
[PULL 15/32] arm/virt: Fix virt_machine_device_plug_cb() error API violation
The Error ** argument must be NULL, &error_abort, &error_fatal, or a
pointer to a variable containing NULL. Passing an argument of the
latter kind twice without clearing it in between is wrong: if the
first call sets an error, it no longer points to NULL for the second
call.
virt_machine_device_plug_cb() passes @errp to
cryptodev_builtin_sym_close_session() in a loop. Harmless, because
cryptodev_builtin_sym_close_session() can't actually fail. Fix by
dropping its Error ** parameter.
Cc: Peter Maydell
Cc: qemu-...@nongnu.org
Signed-off-by: Markus Armbruster
Reviewed-by: Philippe Mathieu-Daudé
Message-Id: <20200422130719.28225-6-arm...@redhat.com>
---
hw/arm/virt.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/hw/arm/virt.c b/hw/arm/virt.c
index 7dc96abf72..cca5316256 100644
--- a/hw/arm/virt.c
+++ b/hw/arm/virt.c
@@ -1186,7 +1186,7 @@ static void create_smmu(const VirtMachineState *vms,
g_free(node);
}
-static void create_virtio_iommu_dt_bindings(VirtMachineState *vms, Error
**errp)
+static void create_virtio_iommu_dt_bindings(VirtMachineState *vms)
{
const char compat[] = "virtio,pci-iommu";
uint16_t bdf = vms->virtio_iommu_bdf;
@@ -2118,7 +2118,7 @@ static void virt_machine_device_plug_cb(HotplugHandler
*hotplug_dev,
vms->iommu = VIRT_IOMMU_VIRTIO;
vms->virtio_iommu_bdf = pci_get_bdf(pdev);
-create_virtio_iommu_dt_bindings(vms, errp);
+create_virtio_iommu_dt_bindings(vms);
}
}
--
2.21.1
[PULL 28/32] smbus: Fix spd_data_generate() for number of banks > 2
spd_data_generate() splits @ram_size bytes into @nbanks RAM banks of
1 << sz_log2 MiB each, like this:
size = ram_size >> 20; /* work in terms of megabytes */
[...]
nbanks = 1;
while (sz_log2 > max_log2 && nbanks < 8) {
sz_log2--;
nbanks++;
}
Each iteration halves the size of a bank, and increments the number of
banks. Wrong: it should double the number of banks.
The bug goes back all the way to commit b296b664ab "smbus: Add a
helper to generate SPD EEPROM data".
It can't bite because spd_data_generate()'s current users pass only
@ram_size that result in *zero* iterations:
machine RAM size#banks typebank size
fulong2e 256 MiB 1 DDR 256 MiB
sam460ex2048 MiB 1 DDR22048 MiB
1024 MiB 1 DDR21024 MiB
512 MiB 1 DDR2 512 MiB
256 MiB 1 DDR2 256 MiB
128 MiB 1 SDR 128 MiB
64 MiB 1 SDR 64 MiB
32 MiB 1 SDR 32 MiB
Apply the obvious, minimal fix. I admit I'm tempted to rip out the
unused (and obviously untested) feature instead, because YAGNI.
Note that this is not the final result, as spd_data_generate() next
increases #banks from 1 to 2 if possible. This is done "to avoid a
bug in MIPS Malta firmware". We don't even use this function with
machine type malta. *Shrug*
Signed-off-by: Markus Armbruster
Message-Id: <20200422134815.1584-5-arm...@redhat.com>
---
hw/i2c/smbus_eeprom.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/hw/i2c/smbus_eeprom.c b/hw/i2c/smbus_eeprom.c
index 07fbbf87f1..e199fc8678 100644
--- a/hw/i2c/smbus_eeprom.c
+++ b/hw/i2c/smbus_eeprom.c
@@ -229,7 +229,7 @@ uint8_t *spd_data_generate(enum sdram_type type, ram_addr_t
ram_size)
nbanks = 1;
while (sz_log2 > max_log2 && nbanks < 8) {
sz_log2--;
-nbanks++;
+nbanks *= 2;
}
assert(size == (1ULL << sz_log2) * nbanks);
--
2.21.1
[PULL 11/32] cryptodev: Fix cryptodev_builtin_cleanup() error API violation
The Error ** argument must be NULL, &error_abort, &error_fatal, or a
pointer to a variable containing NULL. Passing an argument of the
latter kind twice without clearing it in between is wrong: if the
first call sets an error, it no longer points to NULL for the second
call.
cryptodev_builtin_cleanup() passes @errp to
cryptodev_builtin_sym_close_session() in a loop. Harmless, because
cryptodev_builtin_sym_close_session() can't actually fail. Fix it
anyway.
Cc: Gonglei
Signed-off-by: Markus Armbruster
Message-Id: <20200422130719.28225-2-arm...@redhat.com>
---
backends/cryptodev-builtin.c | 10 ++
1 file changed, 2 insertions(+), 8 deletions(-)
diff --git a/backends/cryptodev-builtin.c b/backends/cryptodev-builtin.c
index c8ae3b9742..14316333fe 100644
--- a/backends/cryptodev-builtin.c
+++ b/backends/cryptodev-builtin.c
@@ -282,12 +282,7 @@ static int cryptodev_builtin_sym_close_session(
CryptoDevBackendBuiltin *builtin =
CRYPTODEV_BACKEND_BUILTIN(backend);
-if (session_id >= MAX_NUM_SESSIONS ||
- builtin->sessions[session_id] == NULL) {
-error_setg(errp, "Cannot find a valid session id: %" PRIu64 "",
- session_id);
-return -1;
-}
+assert(session_id < MAX_NUM_SESSIONS && builtin->sessions[session_id]);
qcrypto_cipher_free(builtin->sessions[session_id]->cipher);
g_free(builtin->sessions[session_id]);
@@ -356,8 +351,7 @@ static void cryptodev_builtin_cleanup(
for (i = 0; i < MAX_NUM_SESSIONS; i++) {
if (builtin->sessions[i] != NULL) {
-cryptodev_builtin_sym_close_session(
-backend, i, 0, errp);
+cryptodev_builtin_sym_close_session(backend, i, 0, &error_abort);
}
}
--
2.21.1
[PULL 13/32] cpus: Fix configure_icount() error API violation
The Error ** argument must be NULL, &error_abort, &error_fatal, or a
pointer to a variable containing NULL. Passing an argument of the
latter kind twice without clearing it in between is wrong: if the
first call sets an error, it no longer points to NULL for the second
call.
configure_icount() is wrong that way. Harmless, because its @errp is
always &error_abort or &error_fatal.
Just as wrong (and just as harmless): when it fails, it can still
update global state.
Fix all that.
Cc: Paolo Bonzini
Signed-off-by: Markus Armbruster
Message-Id: <20200422130719.28225-4-arm...@redhat.com>
---
cpus.c | 51 ++-
1 file changed, 30 insertions(+), 21 deletions(-)
diff --git a/cpus.c b/cpus.c
index ef441bdf62..1b542b37f9 100644
--- a/cpus.c
+++ b/cpus.c
@@ -797,40 +797,49 @@ void cpu_ticks_init(void)
void configure_icount(QemuOpts *opts, Error **errp)
{
-const char *option;
+const char *option = qemu_opt_get(opts, "shift");
+bool sleep = qemu_opt_get_bool(opts, "sleep", true);
+bool align = qemu_opt_get_bool(opts, "align", false);
+long time_shift = -1;
char *rem_str = NULL;
-option = qemu_opt_get(opts, "shift");
-if (!option) {
-if (qemu_opt_get(opts, "align") != NULL) {
-error_setg(errp, "Please specify shift option when using align");
-}
+if (!option && qemu_opt_get(opts, "align")) {
+error_setg(errp, "Please specify shift option when using align");
return;
}
-icount_sleep = qemu_opt_get_bool(opts, "sleep", true);
+if (align && !sleep) {
+error_setg(errp, "align=on and sleep=off are incompatible");
+return;
+}
+
+if (strcmp(option, "auto") != 0) {
+errno = 0;
+time_shift = strtol(option, &rem_str, 0);
+if (errno != 0 || *rem_str != '\0' || !strlen(option)) {
+error_setg(errp, "icount: Invalid shift value");
+return;
+}
+} else if (icount_align_option) {
+error_setg(errp, "shift=auto and align=on are incompatible");
+return;
+} else if (!icount_sleep) {
+error_setg(errp, "shift=auto and sleep=off are incompatible");
+return;
+}
+
+icount_sleep = sleep;
if (icount_sleep) {
timers_state.icount_warp_timer = timer_new_ns(QEMU_CLOCK_VIRTUAL_RT,
icount_timer_cb, NULL);
}
-icount_align_option = qemu_opt_get_bool(opts, "align", false);
+icount_align_option = align;
-if (icount_align_option && !icount_sleep) {
-error_setg(errp, "align=on and sleep=off are incompatible");
-}
-if (strcmp(option, "auto") != 0) {
-errno = 0;
-timers_state.icount_time_shift = strtol(option, &rem_str, 0);
-if (errno != 0 || *rem_str != '\0' || !strlen(option)) {
-error_setg(errp, "icount: Invalid shift value");
-}
+if (time_shift >= 0) {
+timers_state.icount_time_shift = time_shift;
use_icount = 1;
return;
-} else if (icount_align_option) {
-error_setg(errp, "shift=auto and align=on are incompatible");
-} else if (!icount_sleep) {
-error_setg(errp, "shift=auto and sleep=off are incompatible");
}
use_icount = 2;
--
2.21.1
[PULL 26/32] smbus: Fix spd_data_generate() error API violation
The Error ** argument must be NULL, &error_abort, &error_fatal, or a
pointer to a variable containing NULL. Passing an argument of the
latter kind twice without clearing it in between is wrong: if the
first call sets an error, it no longer points to NULL for the second
call.
spd_data_generate() can pass @errp to error_setg() more than once when
it adjusts both memory size and type. Harmless, because no caller
passes anything that needs adjusting. Until the previous commit,
sam460ex passed types that needed adjusting, but not sizes.
spd_data_generate()'s contract is rather awkward:
If everything's fine, return non-null and don't set an error.
Else, if memory size or type need adjusting, return non-null and
set an error describing the adjustment.
Else, return null and set an error reporting why no data can be
generated.
Its callers treat the error as a warning even when null is returned.
They don't create the "smbus-eeprom" device then. Suspicious.
Since the previous commit, only "everything's fine" can actually
happen. Drop the unused code and simplify the callers. This gets rid
of the error API violation.
Signed-off-by: Markus Armbruster
Message-Id: <20200422134815.1584-3-arm...@redhat.com>
---
include/hw/i2c/smbus_eeprom.h | 2 +-
hw/i2c/smbus_eeprom.c | 30 --
hw/mips/mips_fulong2e.c | 10 ++
hw/ppc/sam460ex.c | 12 +++-
4 files changed, 10 insertions(+), 44 deletions(-)
diff --git a/include/hw/i2c/smbus_eeprom.h b/include/hw/i2c/smbus_eeprom.h
index 15e2151b50..68b0063ab6 100644
--- a/include/hw/i2c/smbus_eeprom.h
+++ b/include/hw/i2c/smbus_eeprom.h
@@ -31,6 +31,6 @@ void smbus_eeprom_init(I2CBus *bus, int nb_eeprom,
const uint8_t *eeprom_spd, int size);
enum sdram_type { SDR = 0x4, DDR = 0x7, DDR2 = 0x8 };
-uint8_t *spd_data_generate(enum sdram_type type, ram_addr_t size, Error
**errp);
+uint8_t *spd_data_generate(enum sdram_type type, ram_addr_t size);
#endif
diff --git a/hw/i2c/smbus_eeprom.c b/hw/i2c/smbus_eeprom.c
index 5adf3b15b5..07fbbf87f1 100644
--- a/hw/i2c/smbus_eeprom.c
+++ b/hw/i2c/smbus_eeprom.c
@@ -195,8 +195,7 @@ void smbus_eeprom_init(I2CBus *smbus, int nb_eeprom,
}
/* Generate SDRAM SPD EEPROM data describing a module of type and size */
-uint8_t *spd_data_generate(enum sdram_type type, ram_addr_t ram_size,
- Error **errp)
+uint8_t *spd_data_generate(enum sdram_type type, ram_addr_t ram_size)
{
uint8_t *spd;
uint8_t nbanks;
@@ -222,29 +221,10 @@ uint8_t *spd_data_generate(enum sdram_type type,
ram_addr_t ram_size,
g_assert_not_reached();
}
size = ram_size >> 20; /* work in terms of megabytes */
-if (size < 4) {
-error_setg(errp, "SDRAM size is too small");
-return NULL;
-}
sz_log2 = 31 - clz32(size);
size = 1U << sz_log2;
-if (ram_size > size * MiB) {
-error_setg(errp, "SDRAM size 0x"RAM_ADDR_FMT" is not a power of 2, "
- "truncating to %u MB", ram_size, size);
-}
-if (sz_log2 < min_log2) {
-error_setg(errp,
- "Memory size is too small for SDRAM type, adjusting type");
-if (size >= 32) {
-type = DDR;
-min_log2 = 5;
-max_log2 = 12;
-} else {
-type = SDR;
-min_log2 = 2;
-max_log2 = 9;
-}
-}
+assert(ram_size == size * MiB);
+assert(sz_log2 >= min_log2);
nbanks = 1;
while (sz_log2 > max_log2 && nbanks < 8) {
@@ -252,9 +232,7 @@ uint8_t *spd_data_generate(enum sdram_type type, ram_addr_t
ram_size,
nbanks++;
}
-if (size > (1ULL << sz_log2) * nbanks) {
-error_setg(errp, "Memory size is too big for SDRAM, truncating");
-}
+assert(size == (1ULL << sz_log2) * nbanks);
/* split to 2 banks if possible to avoid a bug in MIPS Malta firmware */
if (nbanks == 1 && sz_log2 > min_log2) {
diff --git a/hw/mips/mips_fulong2e.c b/hw/mips/mips_fulong2e.c
index 5040afd581..ef02d54b33 100644
--- a/hw/mips/mips_fulong2e.c
+++ b/hw/mips/mips_fulong2e.c
@@ -297,7 +297,6 @@ static void mips_fulong2e_init(MachineState *machine)
MemoryRegion *bios = g_new(MemoryRegion, 1);
long bios_size;
uint8_t *spd_data;
-Error *err = NULL;
int64_t kernel_entry;
PCIBus *pci_bus;
ISABus *isa_bus;
@@ -377,13 +376,8 @@ static void mips_fulong2e_init(MachineState *machine)
}
/* Populate SPD eeprom data */
-spd_data = spd_data_generate(DDR, machine->ram_size, &err);
-if (err) {
-warn_report_err(err);
-}
-if (spd_data) {
-smbus_eeprom_init_one(smbus, 0x50, spd_data);
-}
+spd_data = spd_data_generate(DDR, machine->ram_size);
+smbus_eeprom_init_one(smbus, 0x50, spd_data);
mc146818_rtc_init(isa_bus, 2000, NULL);
diff --git a/hw/ppc/sam460ex.c b/hw/ppc/sam460ex.c
index 1e3eaac0db..42a
[PULL 23/32] qga: Fix qmp_guest_get_memory_blocks() error handling
The Error ** argument must be NULL, &error_abort, &error_fatal, or a
pointer to a variable containing NULL. Passing an argument of the
latter kind twice without clearing it in between is wrong: if the
first call sets an error, it no longer points to NULL for the second
call.
qmp_guest_get_memory_blocks() passes &local_err to
transfer_memory_block() in a loop. If this fails in more than one
iteration, it can trip error_setv()'s assertion.
Fix it to break the loop.
Cc: Michael Roth
Signed-off-by: Markus Armbruster
Message-Id: <20200422130719.28225-14-arm...@redhat.com>
Reviewed-by: Eric Blake
---
qga/commands-posix.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/qga/commands-posix.c b/qga/commands-posix.c
index a52af0315f..ae1348dc8f 100644
--- a/qga/commands-posix.c
+++ b/qga/commands-posix.c
@@ -2518,6 +2518,9 @@ GuestMemoryBlockList *qmp_guest_get_memory_blocks(Error
**errp)
mem_blk->phys_index = strtoul(&de->d_name[6], NULL, 10);
mem_blk->has_can_offline = true; /* lolspeak ftw */
transfer_memory_block(mem_blk, true, NULL, &local_err);
+if (local_err) {
+break;
+}
entry = g_malloc0(sizeof *entry);
entry->value = mem_blk;
--
2.21.1
[PULL 29/32] Makefile: Drop unused, broken target recurse-fuzz
Target recurse-fuzz depends on pc-bios/optionrom/fuzz, which can't be made. It's not used anywhere. Added in commit c621dc3e01c, looks like cargo cult. Delete. Signed-off-by: Markus Armbruster Message-Id: <20200424071142.3525-2-arm...@redhat.com> Reviewed-by: Alexander Bulekov --- Makefile | 1 - 1 file changed, 1 deletion(-) diff --git a/Makefile b/Makefile index 8a9113e666..34275f57c9 100644 --- a/Makefile +++ b/Makefile @@ -582,7 +582,6 @@ $(ROM_DIRS_RULES): .PHONY: recurse-all recurse-clean recurse-install recurse-all: $(addsuffix /all, $(TARGET_DIRS) $(ROM_DIRS)) -recurse-fuzz: $(addsuffix /fuzz, $(TARGET_DIRS) $(ROM_DIRS)) recurse-clean: $(addsuffix /clean, $(TARGET_DIRS) $(ROM_DIRS)) recurse-install: $(addsuffix /install, $(TARGET_DIRS)) $(addsuffix /install, $(TARGET_DIRS)): all -- 2.21.1
Re: [PATCH] s390x/kvm: help valgrind in several places
On 29.04.20 09:00, Philippe Mathieu-Daudé wrote:
> Hi Christian,
>
> On 4/28/20 8:31 PM, Christian Borntraeger wrote:
>> We need some little help in the code to reduce the valgrind noise.
>> - some designated initializers for the cpu model features and subfunctions
>
> ^ This could go as trivial patch while we discuss the rest.
I can certainly split.
>
>> - mark memory as defined for sida memory reads
>>
>> Signed-off-by: Christian Borntraeger
>> ---
>
> I couldn't apply this patch, then figured out it targets s390-next.
>
>> target/s390x/kvm.c | 15 +--
>> 1 file changed, 13 insertions(+), 2 deletions(-)
>>
>> diff --git a/target/s390x/kvm.c b/target/s390x/kvm.c
>> index 69881a0da0..bcd0ee0d14 100644
>> --- a/target/s390x/kvm.c
>> +++ b/target/s390x/kvm.c
>> @@ -52,6 +52,10 @@
>> #include "hw/s390x/s390-virtio-hcall.h"
>> #include "hw/s390x/pv.h"
>> +#ifdef CONFIG_VALGRIND_H
>> +#include
>> +#endif
>> +
>> #ifndef DEBUG_KVM
>> #define DEBUG_KVM 0
>> #endif
>> @@ -875,6 +879,13 @@ int kvm_s390_mem_op_pv(S390CPU *cpu, uint64_t offset,
>> void *hostbuf,
>> error_report("KVM_S390_MEM_OP failed: %s", strerror(-ret));
>> abort();
>> }
>
> What about kvm_s390_mem_op()?
I have not triggered something in here, but you are right, there should be
cases where we make conditions
depend of that content. Will change my testing and add something here as well.
>
>> +
>> +#ifdef CONFIG_VALGRIND_H
>> + if (!is_write) {
>> + VALGRIND_MAKE_MEM_DEFINED(hostbuf, len);
>> + }
>> +#endif
>
> I agree with this macro usage, but think it should be widely accessible by
> the whole codebase (and other targets).
>
> "exec/memory.h" is for MemoryRegion and AddressSpace. Maybe "exec/ram_addr.h"
> is a better place for common helpers.
>
> If Valgrind is only confused under KVM, the "sysemu/kvm.h" is the obvious
> place.
This macro IS available for the whole codebase if you include
valgrind/memcheck.h.
We used it in the past (before 2.2) for kvm memory.
See commit 541be9274e8ef227fb1b50ce124fd2cc2dce81a5 (kvm/valgrind: don't mark
memory as initialized).
The only thing that we could discuss is introducing a new global function like
valgrind_make_mem_defined that would hide the ifdefs.
Is there interest in such a thing?
It is likely that these corner cases (valgrind not able to see that this is
defined) are more likely
o happen with KVM. But it would be useful for anything not only KVM.
>
>> +
>> return ret;
>> }
>> @@ -2165,7 +2176,7 @@ int kvm_arch_msi_data_to_gsi(uint32_t data)
>> static int query_cpu_subfunc(S390FeatBitmap features)
>> {
>> - struct kvm_s390_vm_cpu_subfunc prop;
>> + struct kvm_s390_vm_cpu_subfunc prop = {};
>> struct kvm_device_attr attr = {
>> .group = KVM_S390_VM_CPU_MODEL,
>> .attr = KVM_S390_VM_CPU_MACHINE_SUBFUNC,
>> @@ -2292,7 +2303,7 @@ static int kvm_to_feat[][2] = {
>> static int query_cpu_feat(S390FeatBitmap features)
>> {
>> - struct kvm_s390_vm_cpu_feat prop;
>> + struct kvm_s390_vm_cpu_feat prop = {};
>> struct kvm_device_attr attr = {
>> .group = KVM_S390_VM_CPU_MODEL,
>> .attr = KVM_S390_VM_CPU_MACHINE_FEAT,
>>
>
[PULL 17/32] bochs-display: Fix vgamem=SIZE error handling
bochs_display_realize() rejects out-of-range vgamem. The error
handling is broken:
$ qemu-system-x86_64 -S -display none -monitor stdio
QEMU 4.2.93 monitor - type 'help' for more information
(qemu) device_add bochs-display,vgamem=1
Error: bochs-display: video memory too small
(qemu) device_add bochs-display,vgamem=1
RAMBlock ":00:04.0/bochs-display-vram" already registered, abort!
Aborted (core dumped)
Cause: bochs_display_realize() neglects to bail out after setting the
error. Fix that.
Fixes: 765c94290863eef1fc4a67819d452cc13b7854a1
Cc: Gerd Hoffmann
Signed-off-by: Markus Armbruster
Reviewed-by: Philippe Mathieu-Daudé
Message-Id: <20200422130719.28225-8-arm...@redhat.com>
Reviewed-by: Gerd Hoffmann
---
hw/display/bochs-display.c | 6 --
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/hw/display/bochs-display.c b/hw/display/bochs-display.c
index 70eb619ef4..e763a0a72d 100644
--- a/hw/display/bochs-display.c
+++ b/hw/display/bochs-display.c
@@ -267,16 +267,18 @@ static void bochs_display_realize(PCIDevice *dev, Error
**errp)
Object *obj = OBJECT(dev);
int ret;
-s->con = graphic_console_init(DEVICE(dev), 0, &bochs_display_gfx_ops, s);
-
if (s->vgamem < 4 * MiB) {
error_setg(errp, "bochs-display: video memory too small");
+return;
}
if (s->vgamem > 256 * MiB) {
error_setg(errp, "bochs-display: video memory too big");
+return;
}
s->vgamem = pow2ceil(s->vgamem);
+s->con = graphic_console_init(DEVICE(dev), 0, &bochs_display_gfx_ops, s);
+
memory_region_init_ram(&s->vram, obj, "bochs-display-vram", s->vgamem,
&error_fatal);
memory_region_init_io(&s->vbe, obj, &bochs_display_vbe_ops, s,
--
2.21.1
[PULL 30/32] fuzz: Simplify how we compute available machines and types
apply_to_qlist(), apply_to_node() work with QObjects. This is
designed for use by tests/qtest/qos-test.c, which gets the data in
that form via QMP. Goes back to commit fc281c8020 "tests: qgraph API
for the qtest driver framework".
Commit 275ab39d86 "fuzz: add support for qos-assisted fuzz targets"
added another user: qtest/fuzz/qos_fuzz.c. To get the data as
QObjects, it uses qmp_marshal_query_machines() and
qmp_marshal_qom_list_types().
All this code is rather cumbersome. Switch to working with generated
QAPI types instead:
* Replace apply_to_qlist() & friends by machines_apply_to_node() and
types_apply_to_node().
* Have qos_fuzz.c use qmp_query_machines() and qmp_qom_list_types()
instead.
* Have qos_test.c convert from QObject to the QAPI types.
Signed-off-by: Markus Armbruster
Message-Id: <20200424071142.3525-3-arm...@redhat.com>
Reviewed-by: Philippe Mathieu-Daudé
Reviewed-by: Alexander Bulekov
---
tests/qtest/libqos/qos_external.h | 8 +++-
tests/qtest/fuzz/qos_fuzz.c | 34 ---
tests/qtest/libqos/qos_external.c | 70 +++
tests/qtest/qos-test.c| 29 +
4 files changed, 59 insertions(+), 82 deletions(-)
diff --git a/tests/qtest/libqos/qos_external.h
b/tests/qtest/libqos/qos_external.h
index 7b44930c55..f63388cb30 100644
--- a/tests/qtest/libqos/qos_external.h
+++ b/tests/qtest/libqos/qos_external.h
@@ -20,8 +20,12 @@
#define QOS_EXTERNAL_H
#include "libqos/qgraph.h"
-void apply_to_node(const char *name, bool is_machine, bool is_abstract);
-void apply_to_qlist(QList *list, bool is_machine);
+#include "libqos/malloc.h"
+#include "qapi/qapi-types-machine.h"
+#include "qapi/qapi-types-qom.h"
+
+void machines_apply_to_node(MachineInfoList *mach_info);
+void types_apply_to_node(ObjectTypeInfoList *type_info);
QGuestAllocator *get_machine_allocator(QOSGraphObject *obj);
void *allocate_objects(QTestState *qts, char **path, QGuestAllocator
**p_alloc);
diff --git a/tests/qtest/fuzz/qos_fuzz.c b/tests/qtest/fuzz/qos_fuzz.c
index af28c92866..87eadb0889 100644
--- a/tests/qtest/fuzz/qos_fuzz.c
+++ b/tests/qtest/fuzz/qos_fuzz.c
@@ -36,7 +36,6 @@
#include "qapi/qapi-commands-machine.h"
#include "qapi/qapi-commands-qom.h"
-#include "qapi/qmp/qlist.h"
void *fuzz_qos_obj;
@@ -45,34 +44,19 @@ QGuestAllocator *fuzz_qos_alloc;
static const char *fuzz_target_name;
static char **fuzz_path_vec;
-/*
- * Replaced the qmp commands with direct qmp_marshal calls.
- * Probably there is a better way to do this
- */
static void qos_set_machines_devices_available(void)
{
-QDict *req = qdict_new();
-QObject *response;
-QDict *args = qdict_new();
-QList *lst;
+MachineInfoList *mach_info;
+ObjectTypeInfoList *type_info;
-qmp_marshal_query_machines(NULL, &response, &error_abort);
-lst = qobject_to(QList, response);
-apply_to_qlist(lst, true);
+mach_info = qmp_query_machines(&error_abort);
+machines_apply_to_node(mach_info);
+qapi_free_MachineInfoList(mach_info);
-qobject_unref(response);
-
-
-qdict_put_str(req, "execute", "qom-list-types");
-qdict_put_str(args, "implements", "device");
-qdict_put_bool(args, "abstract", true);
-qdict_put_obj(req, "arguments", (QObject *) args);
-
-qmp_marshal_qom_list_types(args, &response, &error_abort);
-lst = qobject_to(QList, response);
-apply_to_qlist(lst, false);
-qobject_unref(response);
-qobject_unref(req);
+type_info = qmp_qom_list_types(true, "device", true, true,
+ &error_abort);
+types_apply_to_node(type_info);
+qapi_free_ObjectTypeInfoList(type_info);
}
static char **current_path;
diff --git a/tests/qtest/libqos/qos_external.c
b/tests/qtest/libqos/qos_external.c
index 398556dde0..c707dac3b9 100644
--- a/tests/qtest/libqos/qos_external.c
+++ b/tests/qtest/libqos/qos_external.c
@@ -29,62 +29,40 @@
#include "libqos/qgraph_internal.h"
#include "libqos/qos_external.h"
-
-
-void apply_to_node(const char *name, bool is_machine, bool is_abstract)
+static void machine_apply_to_node(const char *name)
{
-char *machine_name = NULL;
-if (is_machine) {
-const char *arch = qtest_get_arch();
-machine_name = g_strconcat(arch, "/", name, NULL);
-name = machine_name;
+char *machine_name = g_strconcat(qtest_get_arch(), "/", name, NULL);
+
+qos_graph_node_set_availability(machine_name, true);
+g_free(machine_name);
+}
+
+void machines_apply_to_node(MachineInfoList *mach_info)
+{
+MachineInfoList *tail;
+
+for (tail = mach_info; tail; tail = tail->next) {
+machine_apply_to_node(tail->value->name);
+if (tail->value->alias) {
+machine_apply_to_node(tail->value->alias);
+}
}
+}
+
+static void type_apply_to_node(const char *name, bool is_abstract)
+{
qos_graph_node_set_availability(name, true);
if (is_abstract) {
qos_delete_cmd_line(name);
}
-g
[PULL 18/32] virtio-net: Fix duplex=... and speed=... error handling
virtio_net_device_realize() rejects invalid duplex and speed values.
The error handling is broken:
$ ../qemu/bld-sani/x86_64-softmmu/qemu-system-x86_64 -S -display none
-monitor stdio
QEMU 4.2.93 monitor - type 'help' for more information
(qemu) device_add virtio-net,duplex=x
Error: 'duplex' must be 'half' or 'full'
(qemu) c
=
==15654==ERROR: AddressSanitizer: heap-use-after-free on address
0x62e14590 at pc 0x560b75c8dc13 bp 0x7fffdf1a6950 sp 0x7fffdf1a6940
READ of size 8 at 0x62e14590 thread T0
#0 0x560b75c8dc12 in object_dynamic_cast_assert
/work/armbru/qemu/qom/object.c:826
#1 0x560b74c38ac0 in virtio_vmstate_change
/work/armbru/qemu/hw/virtio/virtio.c:3210
#2 0x560b74d9765e in vm_state_notify /work/armbru/qemu/softmmu/vl.c:1271
#3 0x560b7494ba72 in vm_prepare_start /work/armbru/qemu/cpus.c:2156
#4 0x560b7494bacd in vm_start /work/armbru/qemu/cpus.c:2162
#5 0x560b75a7d890 in qmp_cont /work/armbru/qemu/monitor/qmp-cmds.c:160
#6 0x560b75a8d70a in hmp_cont /work/armbru/qemu/monitor/hmp-cmds.c:1043
#7 0x560b75a799f2 in handle_hmp_command
/work/armbru/qemu/monitor/hmp.c:1082
[...]
0x62e14590 is located 33168 bytes inside of 42288-byte region
[0x62e0c400,0x62e16930)
freed by thread T1 here:
#0 0x7feadd39491f in __interceptor_free (/lib64/libasan.so.5+0x10d91f)
#1 0x7feadcebcd7c in g_free (/lib64/libglib-2.0.so.0+0x55d7c)
#2 0x560b75c8fd40 in object_unref /work/armbru/qemu/qom/object.c:1128
#3 0x560b7498a625 in memory_region_unref /work/armbru/qemu/memory.c:1762
#4 0x560b74999fa4 in do_address_space_destroy
/work/armbru/qemu/memory.c:2788
#5 0x560b762362fc in call_rcu_thread /work/armbru/qemu/util/rcu.c:283
#6 0x560b761c8884 in qemu_thread_start
/work/armbru/qemu/util/qemu-thread-posix.c:519
#7 0x7fead9be34bf in start_thread (/lib64/libpthread.so.0+0x84bf)
previously allocated by thread T0 here:
#0 0x7feadd394d18 in __interceptor_malloc (/lib64/libasan.so.5+0x10dd18)
#1 0x7feadcebcc88 in g_malloc (/lib64/libglib-2.0.so.0+0x55c88)
#2 0x560b75c8cf8a in object_new /work/armbru/qemu/qom/object.c:699
#3 0x560b75010ad9 in qdev_device_add
/work/armbru/qemu/qdev-monitor.c:654
#4 0x560b750120c2 in qmp_device_add /work/armbru/qemu/qdev-monitor.c:805
#5 0x560b75012c1b in hmp_device_add /work/armbru/qemu/qdev-monitor.c:905
[...]
==15654==ABORTING
Cause: virtio_net_device_realize() neglects to bail out after setting
the error. Fix that.
Fixes: 9473939ed7addcaaeb8fde5c093918fb7fa0919c
Cc: "Michael S. Tsirkin"
Cc: Jason Wang
Signed-off-by: Markus Armbruster
Reviewed-by: Philippe Mathieu-Daudé
Message-Id: <20200422130719.28225-9-arm...@redhat.com>
Acked-by: Michael S. Tsirkin
---
hw/net/virtio-net.c | 5 -
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
index eddfa7f923..65bb6886c7 100644
--- a/hw/net/virtio-net.c
+++ b/hw/net/virtio-net.c
@@ -2947,6 +2947,7 @@ static void virtio_net_device_realize(DeviceState *dev,
Error **errp)
n->net_conf.duplex = DUPLEX_FULL;
} else {
error_setg(errp, "'duplex' must be 'half' or 'full'");
+return;
}
n->host_features |= (1ULL << VIRTIO_NET_F_SPEED_DUPLEX);
} else {
@@ -2955,7 +2956,9 @@ static void virtio_net_device_realize(DeviceState *dev,
Error **errp)
if (n->net_conf.speed < SPEED_UNKNOWN) {
error_setg(errp, "'speed' must be between 0 and INT_MAX");
-} else if (n->net_conf.speed >= 0) {
+return;
+}
+if (n->net_conf.speed >= 0) {
n->host_features |= (1ULL << VIRTIO_NET_F_SPEED_DUPLEX);
}
--
2.21.1
[PULL 22/32] tests/test-logging: Fix test for -dfilter 0..0xffffffffffffffff
Fixes: 58e19e6e7914354242a67442d0006f9e31684d1a
Signed-off-by: Markus Armbruster
Message-Id: <20200422130719.28225-13-arm...@redhat.com>
Reviewed-by: Philippe Mathieu-Daudé
---
tests/test-logging.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/tests/test-logging.c b/tests/test-logging.c
index 6387e4933f..8580b82420 100644
--- a/tests/test-logging.c
+++ b/tests/test-logging.c
@@ -73,10 +73,10 @@ static void test_parse_range(void)
g_assert(qemu_log_in_addr_range(UINT64_MAX));
g_assert_false(qemu_log_in_addr_range(UINT64_MAX - 1));
-qemu_set_dfilter_ranges("0..0x", &err);
+qemu_set_dfilter_ranges("0..0x", &error_abort);
g_assert(qemu_log_in_addr_range(0));
g_assert(qemu_log_in_addr_range(UINT64_MAX));
-
+
qemu_set_dfilter_ranges("2..1", &err);
error_free_or_abort(&err);
--
2.21.1
[PULL 21/32] migration/colo: Fix qmp_xen_colo_do_checkpoint() error handling
The Error ** argument must be NULL, &error_abort, &error_fatal, or a
pointer to a variable containing NULL. Passing an argument of the
latter kind twice without clearing it in between is wrong: if the
first call sets an error, it no longer points to NULL for the second
call.
qmp_xen_colo_do_checkpoint() passes @errp first to
replication_do_checkpoint_all(), and then to
colo_notify_filters_event(). If both fail, this will trip the
assertion in error_setv().
Similar code in secondary_vm_do_failover() calls
colo_notify_filters_event() only after replication_do_checkpoint_all()
succeeded. Do the same here.
Fixes: 0e8818f023616677416840d6ddc880db8de3c967
Cc: Zhang Chen
Cc: zhanghailiang
Signed-off-by: Markus Armbruster
Reviewed-by: zhanghailiang
Reviewed-by: Philippe Mathieu-Daudé
Reviewed-by: Zhang Chen
Message-Id: <20200422130719.28225-12-arm...@redhat.com>
---
migration/colo.c | 8 +++-
1 file changed, 7 insertions(+), 1 deletion(-)
diff --git a/migration/colo.c b/migration/colo.c
index a54ac84f41..1b3493729b 100644
--- a/migration/colo.c
+++ b/migration/colo.c
@@ -263,7 +263,13 @@ ReplicationStatus *qmp_query_xen_replication_status(Error
**errp)
void qmp_xen_colo_do_checkpoint(Error **errp)
{
-replication_do_checkpoint_all(errp);
+Error *err = NULL;
+
+replication_do_checkpoint_all(&err);
+if (err) {
+error_propagate(errp, err);
+return;
+}
/* Notify all filters of all NIC to do checkpoint */
colo_notify_filters_event(COLO_EVENT_CHECKPOINT, errp);
}
--
2.21.1
Re: [PATCH v1 02/11] hw/arm: versal: Move misplaced comment
On 4/27/20 8:16 PM, Edgar E. Iglesias wrote:
> From: "Edgar E. Iglesias"
>
> Move misplaced comment.
>
> Signed-off-by: Edgar E. Iglesias
Reviewed-by: Luc Michel
> ---
> hw/arm/xlnx-versal.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/hw/arm/xlnx-versal.c b/hw/arm/xlnx-versal.c
> index c73b2fe755..cc696e44c0 100644
> --- a/hw/arm/xlnx-versal.c
> +++ b/hw/arm/xlnx-versal.c
> @@ -36,7 +36,6 @@ static void versal_create_apu_cpus(Versal *s)
>
> obj = object_new(XLNX_VERSAL_ACPU_TYPE);
> if (!obj) {
> -/* Secondary CPUs start in PSCI powered-down state */
> error_report("Unable to create apu.cpu[%d] of type %s",
> i, XLNX_VERSAL_ACPU_TYPE);
> exit(EXIT_FAILURE);
> @@ -49,6 +48,7 @@ static void versal_create_apu_cpus(Versal *s)
> object_property_set_int(obj, s->cfg.psci_conduit,
> "psci-conduit", &error_abort);
> if (i) {
> +/* Secondary CPUs start in PSCI powered-down state */
> object_property_set_bool(obj, true,
> "start-powered-off", &error_abort);
> }
>
Re: [PATCH v5 0/4] introduction of migration_version attribute for VFIO live migration
On Tue, Apr 28, 2020 at 10:14:37PM +0800, Dr. David Alan Gilbert wrote: > * Yan Zhao (yan.y.z...@intel.com) wrote: > > On Mon, Apr 27, 2020 at 11:37:43PM +0800, Dr. David Alan Gilbert wrote: > > > * Yan Zhao (yan.y.z...@intel.com) wrote: > > > > On Sat, Apr 25, 2020 at 03:10:49AM +0800, Dr. David Alan Gilbert wrote: > > > > > * Yan Zhao (yan.y.z...@intel.com) wrote: > > > > > > On Tue, Apr 21, 2020 at 08:08:49PM +0800, Tian, Kevin wrote: > > > > > > > > From: Yan Zhao > > > > > > > > Sent: Tuesday, April 21, 2020 10:37 AM > > > > > > > > > > > > > > > > On Tue, Apr 21, 2020 at 06:56:00AM +0800, Alex Williamson wrote: > > > > > > > > > On Sun, 19 Apr 2020 21:24:57 -0400 > > > > > > > > > Yan Zhao wrote: > > > > > > > > > > > > > > > > > > > On Fri, Apr 17, 2020 at 07:24:57PM +0800, Cornelia Huck > > > > > > > > > > wrote: > > > > > > > > > > > On Fri, 17 Apr 2020 05:52:02 -0400 > > > > > > > > > > > Yan Zhao wrote: > > > > > > > > > > > > > > > > > > > > > > > On Fri, Apr 17, 2020 at 04:44:50PM +0800, Cornelia Huck > > > > > > > > > > > > wrote: > > > > > > > > > > > > > On Mon, 13 Apr 2020 01:52:01 -0400 > > > > > > > > > > > > > Yan Zhao wrote: > > > > > > > > > > > > > > > > > > > > > > > > > > > This patchset introduces a migration_version > > > > > > > > > > > > > > attribute under sysfs > > > > > > > > of VFIO > > > > > > > > > > > > > > Mediated devices. > > > > > > > > > > > > > > > > > > > > > > > > > > > > This migration_version attribute is used to check > > > > > > > > > > > > > > migration > > > > > > > > compatibility > > > > > > > > > > > > > > between two mdev devices. > > > > > > > > > > > > > > > > > > > > > > > > > > > > Currently, it has two locations: > > > > > > > > > > > > > > (1) under mdev_type node, > > > > > > > > > > > > > > which can be used even before device creation, > > > > > > > > > > > > > > but only for > > > > > > > > mdev > > > > > > > > > > > > > > devices of the same mdev type. > > > > > > > > > > > > > > (2) under mdev device node, > > > > > > > > > > > > > > which can only be used after the mdev devices > > > > > > > > > > > > > > are created, but > > > > > > > > the src > > > > > > > > > > > > > > and target mdev devices are not necessarily be > > > > > > > > > > > > > > of the same > > > > > > > > mdev type > > > > > > > > > > > > > > (The second location is newly added in v5, in order > > > > > > > > > > > > > > to keep > > > > > > > > consistent > > > > > > > > > > > > > > with the migration_version node for migratable > > > > > > > > > > > > > > pass-though > > > > > > > > devices) > > > > > > > > > > > > > > > > > > > > > > > > > > What is the relationship between those two attributes? > > > > > > > > > > > > > > > > > > > > > > > > > (1) is for mdev devices specifically, and (2) is > > > > > > > > > > > > provided to keep the > > > > > > > > same > > > > > > > > > > > > sysfs interface as with non-mdev cases. so (2) is for > > > > > > > > > > > > both mdev > > > > > > > > devices and > > > > > > > > > > > > non-mdev devices. > > > > > > > > > > > > > > > > > > > > > > > > in future, if we enable vfio-pci vendor ops, (i.e. a > > > > > > > > > > > > non-mdev device > > > > > > > > > > > > is binding to vfio-pci, but is able to register > > > > > > > > > > > > migration region and do > > > > > > > > > > > > migration transactions from a vendor provided affiliate > > > > > > > > > > > > driver), > > > > > > > > > > > > the vendor driver would export (2) directly, under > > > > > > > > > > > > device node. > > > > > > > > > > > > It is not able to provide (1) as there're no mdev > > > > > > > > > > > > devices involved. > > > > > > > > > > > > > > > > > > > > > > Ok, creating an alternate attribute for non-mdev devices > > > > > > > > > > > makes sense. > > > > > > > > > > > However, wouldn't that rather be a case (3)? The change > > > > > > > > > > > here only > > > > > > > > > > > refers to mdev devices. > > > > > > > > > > > > > > > > > > > > > as you pointed below, (3) and (2) serve the same purpose. > > > > > > > > > > and I think a possible usage is to migrate between a > > > > > > > > > > non-mdev device and > > > > > > > > > > an mdev device. so I think it's better for them both to use > > > > > > > > > > (2) rather > > > > > > > > > > than creating (3). > > > > > > > > > > > > > > > > > > An mdev type is meant to define a software compatible > > > > > > > > > interface, so in > > > > > > > > > the case of mdev->mdev migration, doesn't migrating to a > > > > > > > > > different type > > > > > > > > > fail the most basic of compatibility tests that we expect > > > > > > > > > userspace to > > > > > > > > > perform? IOW, if two mdev types are migration compatible, it > > > > > > > > > seems a > > > > > > > > > prerequisite to that is that they provide the same software > > > > > > > > > interface, > > > > > > > > > which means they should be the same mdev type. > > > > > > > > > > > > > > > > > > In the hybrid ca
Re: [PATCH v1 03/11] hw/arm: versal-virt: Fix typo xlnx-ve -> xlnx-versal
On 4/27/20 8:16 PM, Edgar E. Iglesias wrote: > From: "Edgar E. Iglesias" > > Fix typo xlnx-ve -> xlnx-versal. > > Signed-off-by: Edgar E. Iglesias Reviewed-by: Luc Michel > --- > hw/arm/xlnx-versal-virt.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/hw/arm/xlnx-versal-virt.c b/hw/arm/xlnx-versal-virt.c > index 878a275140..8a608074d1 100644 > --- a/hw/arm/xlnx-versal-virt.c > +++ b/hw/arm/xlnx-versal-virt.c > @@ -440,7 +440,7 @@ static void versal_virt_init(MachineState *machine) > psci_conduit = QEMU_PSCI_CONDUIT_SMC; > } > > -sysbus_init_child_obj(OBJECT(machine), "xlnx-ve", &s->soc, > +sysbus_init_child_obj(OBJECT(machine), "xlnx-versal", &s->soc, >sizeof(s->soc), TYPE_XLNX_VERSAL); > object_property_set_link(OBJECT(&s->soc), OBJECT(machine->ram), > "ddr", &error_abort); >
Re: [PATCH v1 10/11] hw/arm: versal-virt: Add support for SD
On 4/27/20 8:16 PM, Edgar E. Iglesias wrote:
> From: "Edgar E. Iglesias"
>
> Add support for SD.
>
> Signed-off-by: Edgar E. Iglesias
Reviewed-by: Luc Michel
> ---
> hw/arm/xlnx-versal-virt.c | 46 +++
> 1 file changed, 46 insertions(+)
>
> diff --git a/hw/arm/xlnx-versal-virt.c b/hw/arm/xlnx-versal-virt.c
> index d7be1ad494..0afee48672 100644
> --- a/hw/arm/xlnx-versal-virt.c
> +++ b/hw/arm/xlnx-versal-virt.c
> @@ -20,6 +20,7 @@
> #include "hw/arm/sysbus-fdt.h"
> #include "hw/arm/fdt.h"
> #include "cpu.h"
> +#include "hw/qdev-properties.h"
> #include "hw/arm/xlnx-versal.h"
>
> #define TYPE_XLNX_VERSAL_VIRT_MACHINE MACHINE_TYPE_NAME("xlnx-versal-virt")
> @@ -256,6 +257,32 @@ static void fdt_add_zdma_nodes(VersalVirt *s)
> }
> }
>
> +static void fdt_add_sd_nodes(VersalVirt *s)
> +{
> +const char clocknames[] = "clk_xin\0clk_ahb";
> +const char compat[] = "arasan,sdhci-8.9a";
> +int i;
> +
> +for (i = ARRAY_SIZE(s->soc.pmc.iou.sd) - 1; i >= 0; i--) {
> +uint64_t addr = MM_PMC_SD0 + MM_PMC_SD0_SIZE * i;
> +char *name = g_strdup_printf("/sdhci@%" PRIx64, addr);
> +
> +qemu_fdt_add_subnode(s->fdt, name);
> +
> +qemu_fdt_setprop_cells(s->fdt, name, "clocks",
> + s->phandle.clk_25Mhz, s->phandle.clk_25Mhz);
> +qemu_fdt_setprop(s->fdt, name, "clock-names",
> + clocknames, sizeof(clocknames));
> +qemu_fdt_setprop_cells(s->fdt, name, "interrupts",
> + GIC_FDT_IRQ_TYPE_SPI, VERSAL_SD0_IRQ_0 + i *
> 2,
> + GIC_FDT_IRQ_FLAGS_LEVEL_HI);
> +qemu_fdt_setprop_sized_cells(s->fdt, name, "reg",
> + 2, addr, 2, MM_PMC_SD0_SIZE);
> +qemu_fdt_setprop(s->fdt, name, "compatible", compat, sizeof(compat));
> +g_free(name);
> +}
> +}
> +
> static void fdt_nop_memory_nodes(void *fdt, Error **errp)
> {
> Error *err = NULL;
> @@ -411,10 +438,23 @@ static void create_virtio_regions(VersalVirt *s)
> }
> }
>
> +static void sd_plugin_card(SDHCIState *sd, DriveInfo *di)
> +{
> +BlockBackend *blk = di ? blk_by_legacy_dinfo(di) : NULL;
> +DeviceState *card;
> +
> +card = qdev_create(qdev_get_child_bus(DEVICE(sd), "sd-bus"),
> TYPE_SD_CARD);
> +object_property_add_child(OBJECT(sd), "card[*]", OBJECT(card),
> + &error_fatal);
> +qdev_prop_set_drive(card, "drive", blk, &error_fatal);
> +object_property_set_bool(OBJECT(card), true, "realized", &error_fatal);
> +}
> +
> static void versal_virt_init(MachineState *machine)
> {
> VersalVirt *s = XLNX_VERSAL_VIRT_MACHINE(machine);
> int psci_conduit = QEMU_PSCI_CONDUIT_DISABLED;
> +int i;
>
> /*
> * If the user provides an Operating System to be loaded, we expect them
> @@ -455,6 +495,7 @@ static void versal_virt_init(MachineState *machine)
> fdt_add_gic_nodes(s);
> fdt_add_timer_nodes(s);
> fdt_add_zdma_nodes(s);
> +fdt_add_sd_nodes(s);
> fdt_add_cpu_nodes(s, psci_conduit);
> fdt_add_clk_node(s, "/clk125", 12500, s->phandle.clk_125Mhz);
> fdt_add_clk_node(s, "/clk25", 2500, s->phandle.clk_25Mhz);
> @@ -464,6 +505,11 @@ static void versal_virt_init(MachineState *machine)
> memory_region_add_subregion_overlap(get_system_memory(),
> 0, &s->soc.fpd.apu.mr, 0);
>
> +/* Plugin SD cards. */
> +for (i = 0; i < ARRAY_SIZE(s->soc.pmc.iou.sd); i++) {
> +sd_plugin_card(&s->soc.pmc.iou.sd[i], drive_get_next(IF_SD));
> +}
> +
> s->binfo.ram_size = machine->ram_size;
> s->binfo.loader_start = 0x0;
> s->binfo.get_dtb = versal_virt_get_dtb;
>
[PULL 14/32] cpus: Proper range-checking for -icount shift=N
timers_state.icount_time_shift must be in [0,63] to avoid undefined
behavior when shifting by it, e.g. in cpu_icount_to_ns().
icount_adjust() clamps it to [0,MAX_ICOUNT_SHIFT], with
MAX_ICOUNT_SHIFT = 10. configure_icount() doesn't. Fix that.
Fixes: a8bfac37085c3372366d722f131a7e18d664ee4d
Cc: Paolo Bonzini
Signed-off-by: Markus Armbruster
Message-Id: <20200422130719.28225-5-arm...@redhat.com>
---
cpus.c | 7 +++
1 file changed, 3 insertions(+), 4 deletions(-)
diff --git a/cpus.c b/cpus.c
index 1b542b37f9..5670c96bcf 100644
--- a/cpus.c
+++ b/cpus.c
@@ -25,6 +25,7 @@
#include "qemu/osdep.h"
#include "qemu-common.h"
#include "qemu/config-file.h"
+#include "qemu/cutils.h"
#include "migration/vmstate.h"
#include "monitor/monitor.h"
#include "qapi/error.h"
@@ -801,7 +802,6 @@ void configure_icount(QemuOpts *opts, Error **errp)
bool sleep = qemu_opt_get_bool(opts, "sleep", true);
bool align = qemu_opt_get_bool(opts, "align", false);
long time_shift = -1;
-char *rem_str = NULL;
if (!option && qemu_opt_get(opts, "align")) {
error_setg(errp, "Please specify shift option when using align");
@@ -814,9 +814,8 @@ void configure_icount(QemuOpts *opts, Error **errp)
}
if (strcmp(option, "auto") != 0) {
-errno = 0;
-time_shift = strtol(option, &rem_str, 0);
-if (errno != 0 || *rem_str != '\0' || !strlen(option)) {
+if (qemu_strtol(option, NULL, 0, &time_shift) < 0
+|| time_shift < 0 || time_shift > MAX_ICOUNT_SHIFT) {
error_setg(errp, "icount: Invalid shift value");
return;
}
--
2.21.1
[PULL 12/32] block/file-posix: Fix check_cache_dropped() error handling
The Error ** argument must be NULL, &error_abort, &error_fatal, or a
pointer to a variable containing NULL. Passing an argument of the
latter kind twice without clearing it in between is wrong: if the
first call sets an error, it no longer points to NULL for the second
call.
check_cache_dropped() calls error_setg() in a loop. It fails to break
the loop in one instance. If a subsequent iteration error_setg()s
again, it trips error_setv()'s assertion.
Fix it to break the loop.
Fixes: 31be8a2a97ecba7d31a82932286489cac318e9e9
Cc: Stefan Hajnoczi
Signed-off-by: Markus Armbruster
Reviewed-by: Eric Blake
Message-Id: <20200422130719.28225-3-arm...@redhat.com>
---
block/file-posix.c | 5 -
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/block/file-posix.c b/block/file-posix.c
index 7e19bbff5f..094e3b0212 100644
--- a/block/file-posix.c
+++ b/block/file-posix.c
@@ -2691,10 +2691,13 @@ static void check_cache_dropped(BlockDriverState *bs,
Error **errp)
vec_end = DIV_ROUND_UP(length, page_size);
for (i = 0; i < vec_end; i++) {
if (vec[i] & 0x1) {
-error_setg(errp, "page cache still in use!");
break;
}
}
+if (i < vec_end) {
+error_setg(errp, "page cache still in use!");
+break;
+}
}
if (window) {
--
2.21.1
[PATCH v2] audio/jack: add JACK client audiodev
This commit adds a new audiodev backend to allow QEMU to use JACK as
both an audio sink and source.
Signed-off-by: Geoffrey McRae
---
audio/Makefile.objs| 5 +
audio/audio.c | 1 +
audio/audio_template.h | 2 +
audio/jackaudio.c | 583 +
configure | 17 ++
qapi/audio.json| 50 +++-
6 files changed, 656 insertions(+), 2 deletions(-)
create mode 100644 audio/jackaudio.c
diff --git a/audio/Makefile.objs b/audio/Makefile.objs
index d7490a379f..b4a4c11f31 100644
--- a/audio/Makefile.objs
+++ b/audio/Makefile.objs
@@ -28,3 +28,8 @@ common-obj-$(CONFIG_AUDIO_SDL) += sdl.mo
sdl.mo-objs = sdlaudio.o
sdl.mo-cflags := $(SDL_CFLAGS)
sdl.mo-libs := $(SDL_LIBS)
+
+# jack module
+common-obj-$(CONFIG_AUDIO_JACK) += jack.mo
+jack.mo-objs = jackaudio.o
+jack.mo-libs := $(JACK_LIBS)
diff --git a/audio/audio.c b/audio/audio.c
index 7a9e680355..95d9fb16ca 100644
--- a/audio/audio.c
+++ b/audio/audio.c
@@ -1969,6 +1969,7 @@ void audio_create_pdos(Audiodev *dev)
CASE(ALSA, alsa, Alsa);
CASE(COREAUDIO, coreaudio, Coreaudio);
CASE(DSOUND, dsound, );
+CASE(JACK, jack, Jack);
CASE(OSS, oss, Oss);
CASE(PA, pa, Pa);
CASE(SDL, sdl, );
diff --git a/audio/audio_template.h b/audio/audio_template.h
index 7013d3041f..8dd48ce14e 100644
--- a/audio/audio_template.h
+++ b/audio/audio_template.h
@@ -330,6 +330,8 @@ AudiodevPerDirectionOptions *glue(audio_get_pdo_,
TYPE)(Audiodev *dev)
dev->u.coreaudio.TYPE);
case AUDIODEV_DRIVER_DSOUND:
return dev->u.dsound.TYPE;
+case AUDIODEV_DRIVER_JACK:
+return qapi_AudiodevJackPerDirectionOptions_base(dev->u.jack.TYPE);
case AUDIODEV_DRIVER_OSS:
return qapi_AudiodevOssPerDirectionOptions_base(dev->u.oss.TYPE);
case AUDIODEV_DRIVER_PA:
diff --git a/audio/jackaudio.c b/audio/jackaudio.c
new file mode 100644
index 00..0413731044
--- /dev/null
+++ b/audio/jackaudio.c
@@ -0,0 +1,583 @@
+/*
+ * QEMU JACK Audio Connection Kit Client
+ *
+ * Copyright (c) 2020 Geoffrey McRae (gnif)
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a copy
+ * of this software and associated documentation files (the "Software"), to
deal
+ * in the Software without restriction, including without limitation the rights
+ * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
+ * copies of the Software, and to permit persons to whom the Software is
+ * furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice shall be included in
+ * all copies or substantial portions of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
+ * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
FROM,
+ * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
+ * THE SOFTWARE.
+ */
+
+#include "qemu/osdep.h"
+#include "qemu/module.h"
+#include "qemu/fifo8.h"
+#include "qemu-common.h"
+#include "audio.h"
+
+#define AUDIO_CAP "jack"
+#include "audio_int.h"
+
+#include
+#include
+#include
+
+struct QJack;
+
+typedef enum QJackState
+{
+ QJACK_STATE_DISCONNECTED,
+ QJACK_STATE_CONNECTED,
+ QJACK_STATE_IDLE,
+ QJACK_STATE_RUNNING,
+ QJACK_STATE_STOPPING,
+ QJACK_STATE_STOPPED,
+}
+QJackState;
+
+typedef struct QJackBuffer
+{
+ int channels;
+ int frames;
+ _Atomic(int) used;
+ int rptr, wptr;
+ float ** data;
+}
+QJackBuffer;
+
+typedef struct QJackClient
+{
+ bool out;
+ QJackState state;
+ jack_client_t * client;
+ jack_nframes_t freq;
+
+ struct QJack * j;
+ int nchannels;
+ int buffersize;
+ jack_port_t ** port;
+ QJackBuffer fifo;
+}
+QJackClient;
+
+typedef struct QJackOut
+{
+HWVoiceOut hw;
+QJackClient c;
+}
+QJackOut;
+
+typedef struct QJackIn
+{
+HWVoiceIn hw;
+QJackClient c;
+}
+QJackIn;
+
+static void qjack_buffer_create(QJackBuffer * buffer, int channels, int frames)
+{
+buffer->channels = channels;
+buffer->frames = frames;
+buffer->used = 0;
+buffer->rptr = 0;
+buffer->wptr = 0;
+buffer->data = g_malloc(channels * sizeof(float *));
+for(int i = 0; i < channels; ++i)
+buffer->data[i] = g_malloc(frames * sizeof(float));
+}
+
+static void qjack_buffer_clear(QJackBuffer * buffer)
+{
+atomic_store_explicit(&buffer->used, 0, memory_order_relaxed);
+buffer->rptr = 0;
+buffer->wptr = 0;
+}
+
+static void qjack_buffer_free(QJackBuffer * buffer)
+{
+for(int i = 0; i < buffer->channels; ++i)
+g_free(buffer->data[i
Re: [PATCH v1 05/11] hw/arm: versal: Embedd the GEMs into the SoC type
On 4/27/20 8:16 PM, Edgar E. Iglesias wrote:
> From: "Edgar E. Iglesias"
>
> Embedd the GEMs into the SoC type.
>
> Suggested-by: Peter Maydell
> Signed-off-by: Edgar E. Iglesias
Reviewed-by: Luc Michel
> ---
> hw/arm/xlnx-versal.c | 15 ---
> include/hw/arm/xlnx-versal.h | 3 ++-
> 2 files changed, 10 insertions(+), 8 deletions(-)
>
> diff --git a/hw/arm/xlnx-versal.c b/hw/arm/xlnx-versal.c
> index dbde03b7e6..e424aa789e 100644
> --- a/hw/arm/xlnx-versal.c
> +++ b/hw/arm/xlnx-versal.c
> @@ -170,25 +170,26 @@ static void versal_create_gems(Versal *s, qemu_irq *pic)
> DeviceState *dev;
> MemoryRegion *mr;
>
> -dev = qdev_create(NULL, "cadence_gem");
> -s->lpd.iou.gem[i] = SYS_BUS_DEVICE(dev);
> -object_property_add_child(OBJECT(s), name, OBJECT(dev),
> &error_fatal);
> +sysbus_init_child_obj(OBJECT(s), name,
> + &s->lpd.iou.gem[i], sizeof(s->lpd.iou.gem[i]),
> + TYPE_CADENCE_GEM);
> +dev = DEVICE(&s->lpd.iou.gem[i]);
> if (nd->used) {
> qemu_check_nic_model(nd, "cadence_gem");
> qdev_set_nic_properties(dev, nd);
> }
> -object_property_set_int(OBJECT(s->lpd.iou.gem[i]),
> +object_property_set_int(OBJECT(dev),
> 2, "num-priority-queues",
> &error_abort);
> -object_property_set_link(OBJECT(s->lpd.iou.gem[i]),
> +object_property_set_link(OBJECT(dev),
> OBJECT(&s->mr_ps), "dma",
> &error_abort);
> qdev_init_nofail(dev);
>
> -mr = sysbus_mmio_get_region(s->lpd.iou.gem[i], 0);
> +mr = sysbus_mmio_get_region(SYS_BUS_DEVICE(dev), 0);
> memory_region_add_subregion(&s->mr_ps, addrs[i], mr);
>
> -sysbus_connect_irq(s->lpd.iou.gem[i], 0, pic[irqs[i]]);
> +sysbus_connect_irq(SYS_BUS_DEVICE(dev), 0, pic[irqs[i]]);
> g_free(name);
> }
> }
> diff --git a/include/hw/arm/xlnx-versal.h b/include/hw/arm/xlnx-versal.h
> index a3dfd064b3..01da736a5b 100644
> --- a/include/hw/arm/xlnx-versal.h
> +++ b/include/hw/arm/xlnx-versal.h
> @@ -16,6 +16,7 @@
> #include "hw/arm/boot.h"
> #include "hw/intc/arm_gicv3.h"
> #include "hw/char/pl011.h"
> +#include "hw/net/cadence_gem.h"
>
> #define TYPE_XLNX_VERSAL "xlnx-versal"
> #define XLNX_VERSAL(obj) OBJECT_CHECK(Versal, (obj), TYPE_XLNX_VERSAL)
> @@ -51,7 +52,7 @@ typedef struct Versal {
>
> struct {
> PL011State uart[XLNX_VERSAL_NR_UARTS];
> -SysBusDevice *gem[XLNX_VERSAL_NR_GEMS];
> +CadenceGEMState gem[XLNX_VERSAL_NR_GEMS];
> SysBusDevice *adma[XLNX_VERSAL_NR_ADMAS];
> } iou;
> } lpd;
>
Re: [PATCH v1 01/11] hw/arm: versal: Remove inclusion of arm_gicv3_common.h
On 4/27/20 8:16 PM, Edgar E. Iglesias wrote: > From: "Edgar E. Iglesias" > > Remove inclusion of arm_gicv3_common.h, this already gets > included via xlnx-versal.h. > > Signed-off-by: Edgar E. Iglesias Reviewed-by: Luc Michel > --- > hw/arm/xlnx-versal.c | 1 - > 1 file changed, 1 deletion(-) > > diff --git a/hw/arm/xlnx-versal.c b/hw/arm/xlnx-versal.c > index 94460f2343..c73b2fe755 100644 > --- a/hw/arm/xlnx-versal.c > +++ b/hw/arm/xlnx-versal.c > @@ -20,7 +20,6 @@ > #include "hw/arm/boot.h" > #include "kvm_arm.h" > #include "hw/misc/unimp.h" > -#include "hw/intc/arm_gicv3_common.h" > #include "hw/arm/xlnx-versal.h" > #include "hw/char/pl011.h" > >
Re: [PATCH v1 04/11] hw/arm: versal: Embedd the UARTs into the SoC type
On 4/27/20 8:16 PM, Edgar E. Iglesias wrote:
> From: "Edgar E. Iglesias"
>
> Embedd the UARTs into the SoC type.
>
> Suggested-by: Peter Maydell
> Signed-off-by: Edgar E. Iglesias
Hi Edgar,
Just a small thing, I find it easier to review when you have
[diff]
orderFile = scripts/git.orderfile
in your QEMU's .git/config. This way header files come first in the patches.
Reviewed-by: Luc Michel
> ---
> hw/arm/xlnx-versal.c | 12 ++--
> include/hw/arm/xlnx-versal.h | 3 ++-
> 2 files changed, 8 insertions(+), 7 deletions(-)
>
> diff --git a/hw/arm/xlnx-versal.c b/hw/arm/xlnx-versal.c
> index cc696e44c0..dbde03b7e6 100644
> --- a/hw/arm/xlnx-versal.c
> +++ b/hw/arm/xlnx-versal.c
> @@ -21,7 +21,6 @@
> #include "kvm_arm.h"
> #include "hw/misc/unimp.h"
> #include "hw/arm/xlnx-versal.h"
> -#include "hw/char/pl011.h"
>
> #define XLNX_VERSAL_ACPU_TYPE ARM_CPU_TYPE_NAME("cortex-a72")
> #define GEM_REVISION0x40070106
> @@ -144,16 +143,17 @@ static void versal_create_uarts(Versal *s, qemu_irq
> *pic)
> DeviceState *dev;
> MemoryRegion *mr;
>
> -dev = qdev_create(NULL, TYPE_PL011);
> -s->lpd.iou.uart[i] = SYS_BUS_DEVICE(dev);
> +sysbus_init_child_obj(OBJECT(s), name,
> + &s->lpd.iou.uart[i],
> sizeof(s->lpd.iou.uart[i]),
> + TYPE_PL011);
> +dev = DEVICE(&s->lpd.iou.uart[i]);
> qdev_prop_set_chr(dev, "chardev", serial_hd(i));
> -object_property_add_child(OBJECT(s), name, OBJECT(dev),
> &error_fatal);
> qdev_init_nofail(dev);
>
> -mr = sysbus_mmio_get_region(s->lpd.iou.uart[i], 0);
> +mr = sysbus_mmio_get_region(SYS_BUS_DEVICE(dev), 0);
> memory_region_add_subregion(&s->mr_ps, addrs[i], mr);
>
> -sysbus_connect_irq(s->lpd.iou.uart[i], 0, pic[irqs[i]]);
> +sysbus_connect_irq(SYS_BUS_DEVICE(dev), 0, pic[irqs[i]]);
> g_free(name);
> }
> }
> diff --git a/include/hw/arm/xlnx-versal.h b/include/hw/arm/xlnx-versal.h
> index 6c0a692b2f..a3dfd064b3 100644
> --- a/include/hw/arm/xlnx-versal.h
> +++ b/include/hw/arm/xlnx-versal.h
> @@ -15,6 +15,7 @@
> #include "hw/sysbus.h"
> #include "hw/arm/boot.h"
> #include "hw/intc/arm_gicv3.h"
> +#include "hw/char/pl011.h"
>
> #define TYPE_XLNX_VERSAL "xlnx-versal"
> #define XLNX_VERSAL(obj) OBJECT_CHECK(Versal, (obj), TYPE_XLNX_VERSAL)
> @@ -49,7 +50,7 @@ typedef struct Versal {
> MemoryRegion mr_ocm;
>
> struct {
> -SysBusDevice *uart[XLNX_VERSAL_NR_UARTS];
> +PL011State uart[XLNX_VERSAL_NR_UARTS];
> SysBusDevice *gem[XLNX_VERSAL_NR_GEMS];
> SysBusDevice *adma[XLNX_VERSAL_NR_ADMAS];
> } iou;
>
[PATCH v2] s390x/kvm: help valgrind in several places
We need some little help in the code to reduce the valgrind noise.
This patch does this with some designated initializers for the cpu
model features and subfunctions.
Reviewed-by: Philippe Mathieu-Daudé
Signed-off-by: Christian Borntraeger
---
target/s390x/kvm.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/target/s390x/kvm.c b/target/s390x/kvm.c
index 69881a0da0..f2f75d2a57 100644
--- a/target/s390x/kvm.c
+++ b/target/s390x/kvm.c
@@ -2165,7 +2165,7 @@ int kvm_arch_msi_data_to_gsi(uint32_t data)
static int query_cpu_subfunc(S390FeatBitmap features)
{
-struct kvm_s390_vm_cpu_subfunc prop;
+struct kvm_s390_vm_cpu_subfunc prop = {};
struct kvm_device_attr attr = {
.group = KVM_S390_VM_CPU_MODEL,
.attr = KVM_S390_VM_CPU_MACHINE_SUBFUNC,
@@ -2292,7 +2292,7 @@ static int kvm_to_feat[][2] = {
static int query_cpu_feat(S390FeatBitmap features)
{
-struct kvm_s390_vm_cpu_feat prop;
+struct kvm_s390_vm_cpu_feat prop = {};
struct kvm_device_attr attr = {
.group = KVM_S390_VM_CPU_MODEL,
.attr = KVM_S390_VM_CPU_MACHINE_FEAT,
--
2.25.1
Re: [PATCH V2] Add a new PIIX option to control PCI hot unplugging of devices on non-root buses
> On Apr 29, 2020, at 1:08 PM, Michael S. Tsirkin wrote: > > On Wed, Apr 29, 2020 at 07:02:56AM +, Ani Sinha wrote: >> >> >>> On Apr 29, 2020, at 12:27 PM, Michael S. Tsirkin wrote: >>> >>> On Wed, Apr 29, 2020 at 06:54:52AM +, Ani Sinha wrote: > On Apr 29, 2020, at 12:22 PM, Michael S. Tsirkin wrote: > > On Wed, Apr 29, 2020 at 06:11:20AM +, Ani Sinha wrote: >> >> >>> On Apr 29, 2020, at 10:58 AM, Michael S. Tsirkin >>> wrote: >>> >>> o if there's a need to disable >>> just one of these, commit log needs to do a better job documenting the >>> usecase. >> >> The use case is simple. With this feature admins will be able to do what >> they were forced to do from Windows driver level but now at the bus >> level. Hence, >> (a) They need not have access to the guest VM to change or update >> windows driver registry settings. They can enable the same setting from >> admin management console without any access to VM. >> (b) It is more robust. No need to mess with driver settings. Incorrect >> settings can brick guest OS. Also no guest specific knowhow required. >> (c) It is more scalable since a single cluster wide setting can be used >> for all VM power ons and the management plane can take care of the rest >> automatically. No need to access individual VMs to enforce this. >> (d) I am told that the driver level solution does not persist across a >> reboot. >> >> Ani > > Looks like disabling both plug and unplug would also address these needs. No the driver level solution does not prevent hot plugging of devices but blocks just hot unplugging. The solution I am proposing tries to do the same but from the bus/hypervisor level. >>> >>> Why does the driver level solution need to prevent just hot unplugging? >> >> Because it not fair to prevent end users from hot plugging new devices when >> it is the (accidental?) hot unplugging of existing devices which causes >> issues. > > Accidental? So maybe what you need is actually something else then - > avoid *removing* the device when it's powered down. You don’t get it. It is not hypervisor admins who are unplugging it. It is the end users. Even RedHat customers want this feature. See following resources: https://www.redhat.com/archives/libvir-list/2020-February/msg00110.html https://bugzilla.redhat.com/show_bug.cgi?id=1802592 https://bugzilla.redhat.com/show_bug.cgi?id=1790899 My approach is much more fine grained than just disable everything approach that we have for q35. For i440fx we can do better than that. > >>> >>> > -- > MST
Re: [PATCH 1/4] block: Add bdrv_make_empty()
On 28.04.20 16:21, Kevin Wolf wrote:
> Am 28.04.2020 um 15:26 hat Max Reitz geschrieben:
>> Right now, all users of bdrv_make_empty() call the BlockDriver method
>> directly. That is not only bad style, it is also wrong, unless the
>> caller has a BdrvChild with a WRITE permission.
>>
>> Introduce bdrv_make_empty() that verifies that it does.
>>
>> Signed-off-by: Max Reitz
>> ---
>> include/block/block.h | 1 +
>> block.c | 23 +++
>> 2 files changed, 24 insertions(+)
>>
>> diff --git a/include/block/block.h b/include/block/block.h
>> index b05995fe9c..d947fb4080 100644
>> --- a/include/block/block.h
>> +++ b/include/block/block.h
>> @@ -351,6 +351,7 @@ BlockMeasureInfo *bdrv_measure(BlockDriver *drv,
>> QemuOpts *opts,
>> void bdrv_get_geometry(BlockDriverState *bs, uint64_t *nb_sectors_ptr);
>> void bdrv_refresh_limits(BlockDriverState *bs, Error **errp);
>> int bdrv_commit(BlockDriverState *bs);
>> +int bdrv_make_empty(BdrvChild *c, Error **errp);
>> int bdrv_change_backing_file(BlockDriverState *bs,
>> const char *backing_file, const char *backing_fmt);
>> void bdrv_register(BlockDriver *bdrv);
>> diff --git a/block.c b/block.c
>> index 2e3905c99e..b0d5b98617 100644
>> --- a/block.c
>> +++ b/block.c
>> @@ -6791,3 +6791,26 @@ void bdrv_del_child(BlockDriverState *parent_bs,
>> BdrvChild *child, Error **errp)
>>
>> parent_bs->drv->bdrv_del_child(parent_bs, child, errp);
>> }
>> +
>> +int bdrv_make_empty(BdrvChild *c, Error **errp)
>> +{
>> +BlockDriver *drv = c->bs->drv;
>> +int ret;
>> +
>> +assert(c->perm & BLK_PERM_WRITE);
>
> If I understand correctly, bdrv_make_empty() is called to drop an
> overlay whose content is identical to what it would read from its
> backing file (in particular after a commit operation). This means that
> the caller promises that the visible content doesn't change.
>
> So should we check BLK_PERM_WRITE_UNCHANGED instead?
Ah, right. Yes, that would be better. (Or to check both, whether any
of them has been taken.)
Max
signature.asc
Description: OpenPGP digital signature
Re: [PATCH 07/11] mips/malta: Fix create_cps() error handling
Philippe Mathieu-Daudé writes:
> +Peter for crediting his advice.
>
> On 4/29/20 7:59 AM, Markus Armbruster wrote:
>> Philippe Mathieu-Daudé writes:
>>
>>> On 4/24/20 9:20 PM, Markus Armbruster wrote:
The Error ** argument must be NULL, &error_abort, &error_fatal, or a
pointer to a variable containing NULL. Passing an argument of the
latter kind twice without clearing it in between is wrong: if the
first call sets an error, it no longer points to NULL for the second
create_cps() is wrong that way. The last calls treats an error as
fatal. Do that for the prior ones, too.
Fixes: bff384a4fbd5d0e86939092e74e766ef0f5f592c
Cc: Aleksandar Markovic
Cc: "Philippe Mathieu-Daudé"
Cc: Aurelien Jarno
Signed-off-by: Markus Armbruster
---
hw/mips/mips_malta.c | 15 ++-
1 file changed, 6 insertions(+), 9 deletions(-)
diff --git a/hw/mips/mips_malta.c b/hw/mips/mips_malta.c
index e4c4de1b4e..17bf41616b 100644
--- a/hw/mips/mips_malta.c
+++ b/hw/mips/mips_malta.c
@@ -1185,17 +1185,14 @@ static void create_cpu_without_cps(MachineState
*ms,
static void create_cps(MachineState *ms, MaltaState *s,
qemu_irq *cbus_irq, qemu_irq *i8259_irq)
{
-Error *err = NULL;
-
sysbus_init_child_obj(OBJECT(s), "cps", OBJECT(&s->cps),
sizeof(s->cps),
TYPE_MIPS_CPS);
-object_property_set_str(OBJECT(&s->cps), ms->cpu_type, "cpu-type",
&err);
-object_property_set_int(OBJECT(&s->cps), ms->smp.cpus, "num-vp",
&err);
-object_property_set_bool(OBJECT(&s->cps), true, "realized", &err);
-if (err != NULL) {
-error_report("%s", error_get_pretty(err));
>>>
>>> In https://www.mail-archive.com/qemu-devel@nongnu.org/msg695645.html I
>>> also remove "qemu/error-report.h" which is not needed once you remove
>>> this call.
>>
>> Missed it, sorry. I only reviewed the Coccinelle scripts [PATCH 1+3/7].
>
> My bad for not Cc'ing you on the whole series, which is Error related,
> and use the default get_maintainer.pl selection.
>
>> I'd replace my patch by yours to give you proper credit, but your commit
>> message mentions "the coccinelle script", presumably the one from PATCH
>> 1/7, and that patch isn't quite ready in my opinion.
>
> I'm not worried about credit, but duplicating effort or wasting time.
>
> Peter once warned the problem with Coccinelle scripts is finding the
> correct balance between time spent to improve QEMU codebase, and time
> spent learning Coccinelle and improving a script that is often used
> only once in a lifetime.
> If the script is not provided, we ask for the script. If the script is
> embedded in various patch descriptions, we ask to add it independently
> for reuse or as example. Then the script must be almost
> perfect. Meanwhile all the following patches referencing it, while
> reviewed, are stuck.
True. I try not to ask for undue perfection, but perfection eludes me
in that, too :)
For PATCH 1/7, I only asked you to explain the script's limitations in
the script and the commit message. Her's a bit of inspiration from the
kernel's scripts/coccinelle/misc/doubleinit.cocci (picked almost at
random):
/// Find duplicate field initializations. This has a high rate of false
/// positives due to #ifdefs, which Coccinelle is not aware of in a
structure
/// initialization.
///
// Confidence: Low
I like the Confidence: tag. It should come with an explanation, as it
does here.
For PATCH 3/7, I had a question.
> Anyway back to your patch:
> Reviewed-by: Philippe Mathieu-Daudé
> Tested-by: Philippe Mathieu-Daudé
Thanks!
Re: [PATCH v1 07/11] hw/arm: versal: Embedd the APUs into the SoC type
On 4/27/20 8:16 PM, Edgar E. Iglesias wrote:
> From: "Edgar E. Iglesias"
>
> Embedd the APUs into the SoC type.
>
> Suggested-by: Peter Maydell
> Signed-off-by: Edgar E. Iglesias
Reviewed-by: Luc Michel
> ---
> hw/arm/xlnx-versal-virt.c| 4 ++--
> hw/arm/xlnx-versal.c | 19 +--
> include/hw/arm/xlnx-versal.h | 2 +-
> 3 files changed, 8 insertions(+), 17 deletions(-)
>
> diff --git a/hw/arm/xlnx-versal-virt.c b/hw/arm/xlnx-versal-virt.c
> index 8a608074d1..d7be1ad494 100644
> --- a/hw/arm/xlnx-versal-virt.c
> +++ b/hw/arm/xlnx-versal-virt.c
> @@ -469,9 +469,9 @@ static void versal_virt_init(MachineState *machine)
> s->binfo.get_dtb = versal_virt_get_dtb;
> s->binfo.modify_dtb = versal_virt_modify_dtb;
> if (machine->kernel_filename) {
> -arm_load_kernel(s->soc.fpd.apu.cpu[0], machine, &s->binfo);
> +arm_load_kernel(&s->soc.fpd.apu.cpu[0], machine, &s->binfo);
> } else {
> -AddressSpace *as = arm_boot_address_space(s->soc.fpd.apu.cpu[0],
> +AddressSpace *as = arm_boot_address_space(&s->soc.fpd.apu.cpu[0],
>&s->binfo);
> /* Some boot-loaders (e.g u-boot) don't like blobs at address 0
> (NULL).
> * Offset things by 4K. */
> diff --git a/hw/arm/xlnx-versal.c b/hw/arm/xlnx-versal.c
> index ebd2dc51be..c8a296e2e0 100644
> --- a/hw/arm/xlnx-versal.c
> +++ b/hw/arm/xlnx-versal.c
> @@ -31,19 +31,11 @@ static void versal_create_apu_cpus(Versal *s)
>
> for (i = 0; i < ARRAY_SIZE(s->fpd.apu.cpu); i++) {
> Object *obj;
> -char *name;
> -
> -obj = object_new(XLNX_VERSAL_ACPU_TYPE);
> -if (!obj) {
> -error_report("Unable to create apu.cpu[%d] of type %s",
> - i, XLNX_VERSAL_ACPU_TYPE);
> -exit(EXIT_FAILURE);
> -}
> -
> -name = g_strdup_printf("apu-cpu[%d]", i);
> -object_property_add_child(OBJECT(s), name, obj, &error_fatal);
> -g_free(name);
>
> +object_initialize_child(OBJECT(s), "apu-cpu[*]",
> +&s->fpd.apu.cpu[i],
> sizeof(s->fpd.apu.cpu[i]),
> +XLNX_VERSAL_ACPU_TYPE, &error_abort, NULL);
> +obj = OBJECT(&s->fpd.apu.cpu[i]);
> object_property_set_int(obj, s->cfg.psci_conduit,
> "psci-conduit", &error_abort);
> if (i) {
> @@ -57,7 +49,6 @@ static void versal_create_apu_cpus(Versal *s)
> object_property_set_link(obj, OBJECT(&s->fpd.apu.mr), "memory",
> &error_abort);
> object_property_set_bool(obj, true, "realized", &error_fatal);
> -s->fpd.apu.cpu[i] = ARM_CPU(obj);
> }
> }
>
> @@ -95,7 +86,7 @@ static void versal_create_apu_gic(Versal *s, qemu_irq *pic)
> }
>
> for (i = 0; i < nr_apu_cpus; i++) {
> -DeviceState *cpudev = DEVICE(s->fpd.apu.cpu[i]);
> +DeviceState *cpudev = DEVICE(&s->fpd.apu.cpu[i]);
> int ppibase = XLNX_VERSAL_NR_IRQS + i * GIC_INTERNAL + GIC_NR_SGIS;
> qemu_irq maint_irq;
> int ti;
> diff --git a/include/hw/arm/xlnx-versal.h b/include/hw/arm/xlnx-versal.h
> index 94b7826fd4..426b66449d 100644
> --- a/include/hw/arm/xlnx-versal.h
> +++ b/include/hw/arm/xlnx-versal.h
> @@ -36,7 +36,7 @@ typedef struct Versal {
> struct {
> struct {
> MemoryRegion mr;
> -ARMCPU *cpu[XLNX_VERSAL_NR_ACPUS];
> +ARMCPU cpu[XLNX_VERSAL_NR_ACPUS];
> GICv3State gic;
> } apu;
> } fpd;
>
Re: [PATCH v2] s390x/kvm: help valgrind in several places
On 29.04.20 09:42, Christian Borntraeger wrote:
> We need some little help in the code to reduce the valgrind noise.
> This patch does this with some designated initializers for the cpu
> model features and subfunctions.
>
> Reviewed-by: Philippe Mathieu-Daudé
> Signed-off-by: Christian Borntraeger
> ---
> target/s390x/kvm.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/target/s390x/kvm.c b/target/s390x/kvm.c
> index 69881a0da0..f2f75d2a57 100644
> --- a/target/s390x/kvm.c
> +++ b/target/s390x/kvm.c
> @@ -2165,7 +2165,7 @@ int kvm_arch_msi_data_to_gsi(uint32_t data)
>
> static int query_cpu_subfunc(S390FeatBitmap features)
> {
> -struct kvm_s390_vm_cpu_subfunc prop;
> +struct kvm_s390_vm_cpu_subfunc prop = {};
> struct kvm_device_attr attr = {
> .group = KVM_S390_VM_CPU_MODEL,
> .attr = KVM_S390_VM_CPU_MACHINE_SUBFUNC,
> @@ -2292,7 +2292,7 @@ static int kvm_to_feat[][2] = {
>
> static int query_cpu_feat(S390FeatBitmap features)
> {
> -struct kvm_s390_vm_cpu_feat prop;
> +struct kvm_s390_vm_cpu_feat prop = {};
> struct kvm_device_attr attr = {
> .group = KVM_S390_VM_CPU_MODEL,
> .attr = KVM_S390_VM_CPU_MACHINE_FEAT,
>
Reviewed-by: David Hildenbrand
--
Thanks,
David / dhildenb
Re: [PATCH for-5.1 0/5] qobject: Minor spring cleaning
Queued.
Re: [PATCH v1 06/11] hw/arm: versal: Embedd the ADMAs into the SoC type
On 4/27/20 8:16 PM, Edgar E. Iglesias wrote:
> From: "Edgar E. Iglesias"
>
> Embedd the ADMAs into the SoC type.
>
> Suggested-by: Peter Maydell
> Signed-off-by: Edgar E. Iglesias
Reviewed-by: Luc Michel
> ---
> hw/arm/xlnx-versal.c | 14 +++---
> include/hw/arm/xlnx-versal.h | 3 ++-
> 2 files changed, 9 insertions(+), 8 deletions(-)
>
> diff --git a/hw/arm/xlnx-versal.c b/hw/arm/xlnx-versal.c
> index e424aa789e..ebd2dc51be 100644
> --- a/hw/arm/xlnx-versal.c
> +++ b/hw/arm/xlnx-versal.c
> @@ -203,18 +203,18 @@ static void versal_create_admas(Versal *s, qemu_irq
> *pic)
> DeviceState *dev;
> MemoryRegion *mr;
>
> -dev = qdev_create(NULL, "xlnx.zdma");
> -s->lpd.iou.adma[i] = SYS_BUS_DEVICE(dev);
> -object_property_set_int(OBJECT(s->lpd.iou.adma[i]), 128, "bus-width",
> -&error_abort);
> -object_property_add_child(OBJECT(s), name, OBJECT(dev),
> &error_fatal);
> +sysbus_init_child_obj(OBJECT(s), name,
> + &s->lpd.iou.adma[i],
> sizeof(s->lpd.iou.adma[i]),
> + TYPE_XLNX_ZDMA);
> +dev = DEVICE(&s->lpd.iou.adma[i]);
> +object_property_set_int(OBJECT(dev), 128, "bus-width", &error_abort);
> qdev_init_nofail(dev);
>
> -mr = sysbus_mmio_get_region(s->lpd.iou.adma[i], 0);
> +mr = sysbus_mmio_get_region(SYS_BUS_DEVICE(dev), 0);
> memory_region_add_subregion(&s->mr_ps,
> MM_ADMA_CH0 + i * MM_ADMA_CH0_SIZE, mr);
>
> -sysbus_connect_irq(s->lpd.iou.adma[i], 0, pic[VERSAL_ADMA_IRQ_0 +
> i]);
> +sysbus_connect_irq(SYS_BUS_DEVICE(dev), 0, pic[VERSAL_ADMA_IRQ_0 +
> i]);
> g_free(name);
> }
> }
> diff --git a/include/hw/arm/xlnx-versal.h b/include/hw/arm/xlnx-versal.h
> index 01da736a5b..94b7826fd4 100644
> --- a/include/hw/arm/xlnx-versal.h
> +++ b/include/hw/arm/xlnx-versal.h
> @@ -16,6 +16,7 @@
> #include "hw/arm/boot.h"
> #include "hw/intc/arm_gicv3.h"
> #include "hw/char/pl011.h"
> +#include "hw/dma/xlnx-zdma.h"
> #include "hw/net/cadence_gem.h"
>
> #define TYPE_XLNX_VERSAL "xlnx-versal"
> @@ -53,7 +54,7 @@ typedef struct Versal {
> struct {
> PL011State uart[XLNX_VERSAL_NR_UARTS];
> CadenceGEMState gem[XLNX_VERSAL_NR_GEMS];
> -SysBusDevice *adma[XLNX_VERSAL_NR_ADMAS];
> +XlnxZDMA adma[XLNX_VERSAL_NR_ADMAS];
> } iou;
> } lpd;
>
>
Re: [PATCH v1 09/11] hw/arm: versal: Add support for the RTC
On 4/27/20 8:16 PM, Edgar E. Iglesias wrote:
> From: "Edgar E. Iglesias"
>
> hw/arm: versal: Add support for the RTC.
>
> Signed-off-by: Edgar E. Iglesias
Reviewed-by: Luc Michel
> ---
> hw/arm/xlnx-versal.c | 21 +
> include/hw/arm/xlnx-versal.h | 8
> 2 files changed, 29 insertions(+)
>
> diff --git a/hw/arm/xlnx-versal.c b/hw/arm/xlnx-versal.c
> index e263bdf77a..321171bcce 100644
> --- a/hw/arm/xlnx-versal.c
> +++ b/hw/arm/xlnx-versal.c
> @@ -240,6 +240,26 @@ static void versal_create_sds(Versal *s, qemu_irq *pic)
> }
> }
>
> +static void versal_create_rtc(Versal *s, qemu_irq *pic)
> +{
> +SysBusDevice *sbd;
> +MemoryRegion *mr;
> +
> +sysbus_init_child_obj(OBJECT(s), "rtc", &s->pmc.rtc, sizeof(s->pmc.rtc),
> + TYPE_XLNX_ZYNQMP_RTC);
> +sbd = SYS_BUS_DEVICE(&s->pmc.rtc);
> +qdev_init_nofail(DEVICE(sbd));
> +
> +mr = sysbus_mmio_get_region(sbd, 0);
> +memory_region_add_subregion(&s->mr_ps, MM_PMC_RTC, mr);
> +
> +/*
> + * TODO: Connect the ALARM and SECONDS interrupts once our RTC model
> + * supports them.
> + */
> +sysbus_connect_irq(sbd, 1, pic[VERSAL_RTC_APB_ERR_IRQ]);
> +}
> +
> /* This takes the board allocated linear DDR memory and creates aliases
> * for each split DDR range/aperture on the Versal address map.
> */
> @@ -323,6 +343,7 @@ static void versal_realize(DeviceState *dev, Error **errp)
> versal_create_gems(s, pic);
> versal_create_admas(s, pic);
> versal_create_sds(s, pic);
> +versal_create_rtc(s, pic);
> versal_map_ddr(s);
> versal_unimp(s);
>
> diff --git a/include/hw/arm/xlnx-versal.h b/include/hw/arm/xlnx-versal.h
> index e11693e29d..9c9f47ba9d 100644
> --- a/include/hw/arm/xlnx-versal.h
> +++ b/include/hw/arm/xlnx-versal.h
> @@ -19,6 +19,7 @@
> #include "hw/char/pl011.h"
> #include "hw/dma/xlnx-zdma.h"
> #include "hw/net/cadence_gem.h"
> +#include "hw/rtc/xlnx-zynqmp-rtc.h"
>
> #define TYPE_XLNX_VERSAL "xlnx-versal"
> #define XLNX_VERSAL(obj) OBJECT_CHECK(Versal, (obj), TYPE_XLNX_VERSAL)
> @@ -65,6 +66,8 @@ typedef struct Versal {
> struct {
> SDHCIState sd[XLNX_VERSAL_NR_SDS];
> } iou;
> +
> +XlnxZynqMPRTC rtc;
> } pmc;
>
> struct {
> @@ -89,7 +92,10 @@ typedef struct Versal {
> #define VERSAL_GEM1_IRQ_0 58
> #define VERSAL_GEM1_WAKE_IRQ_0 59
> #define VERSAL_ADMA_IRQ_0 60
> +#define VERSAL_RTC_APB_ERR_IRQ 121
> #define VERSAL_SD0_IRQ_0 126
> +#define VERSAL_RTC_ALARM_IRQ 142
> +#define VERSAL_RTC_SECONDS_IRQ 143
>
> /* Architecturally reserved IRQs suitable for virtualization. */
> #define VERSAL_RSVD_IRQ_FIRST 111
> @@ -143,4 +149,6 @@ typedef struct Versal {
> #define MM_PMC_SD0_SIZE 0x1
> #define MM_PMC_CRP 0xf126U
> #define MM_PMC_CRP_SIZE 0x1
> +#define MM_PMC_RTC 0xf12a
> +#define MM_PMC_RTC_SIZE 0x1
> #endif
>
Re: [PATCH v1 08/11] hw/arm: versal: Add support for SD
On 4/27/20 8:16 PM, Edgar E. Iglesias wrote:
> From: "Edgar E. Iglesias"
>
> Add support for SD.
>
> Signed-off-by: Edgar E. Iglesias
Reviewed-by: Luc Michel
> ---
> hw/arm/xlnx-versal.c | 31 +++
> include/hw/arm/xlnx-versal.h | 12
> 2 files changed, 43 insertions(+)
>
> diff --git a/hw/arm/xlnx-versal.c b/hw/arm/xlnx-versal.c
> index c8a296e2e0..e263bdf77a 100644
> --- a/hw/arm/xlnx-versal.c
> +++ b/hw/arm/xlnx-versal.c
> @@ -210,6 +210,36 @@ static void versal_create_admas(Versal *s, qemu_irq *pic)
> }
> }
>
> +#define SDHCI_CAPABILITIES 0x280737ec6481 /* Same as on ZynqMP. */
> +static void versal_create_sds(Versal *s, qemu_irq *pic)
> +{
> +int i;
> +
> +for (i = 0; i < ARRAY_SIZE(s->pmc.iou.sd); i++) {
> +DeviceState *dev;
> +MemoryRegion *mr;
> +
> +sysbus_init_child_obj(OBJECT(s), "sd[*]",
> + &s->pmc.iou.sd[i], sizeof(s->pmc.iou.sd[i]),
> + TYPE_SYSBUS_SDHCI);
> +dev = DEVICE(&s->pmc.iou.sd[i]);
> +
> +object_property_set_uint(OBJECT(dev),
> + 3, "sd-spec-version", &error_fatal);
> +object_property_set_uint(OBJECT(dev), SDHCI_CAPABILITIES, "capareg",
> + &error_fatal);
> +object_property_set_uint(OBJECT(dev), UHS_I, "uhs", &error_fatal);
> +qdev_init_nofail(dev);
> +
> +mr = sysbus_mmio_get_region(SYS_BUS_DEVICE(dev), 0);
> +memory_region_add_subregion(&s->mr_ps,
> +MM_PMC_SD0 + i * MM_PMC_SD0_SIZE, mr);
> +
> +sysbus_connect_irq(SYS_BUS_DEVICE(dev), 0,
> + pic[VERSAL_SD0_IRQ_0 + i * 2]);
> +}
> +}
> +
> /* This takes the board allocated linear DDR memory and creates aliases
> * for each split DDR range/aperture on the Versal address map.
> */
> @@ -292,6 +322,7 @@ static void versal_realize(DeviceState *dev, Error **errp)
> versal_create_uarts(s, pic);
> versal_create_gems(s, pic);
> versal_create_admas(s, pic);
> +versal_create_sds(s, pic);
> versal_map_ddr(s);
> versal_unimp(s);
>
> diff --git a/include/hw/arm/xlnx-versal.h b/include/hw/arm/xlnx-versal.h
> index 426b66449d..e11693e29d 100644
> --- a/include/hw/arm/xlnx-versal.h
> +++ b/include/hw/arm/xlnx-versal.h
> @@ -14,6 +14,7 @@
>
> #include "hw/sysbus.h"
> #include "hw/arm/boot.h"
> +#include "hw/sd/sdhci.h"
> #include "hw/intc/arm_gicv3.h"
> #include "hw/char/pl011.h"
> #include "hw/dma/xlnx-zdma.h"
> @@ -26,6 +27,7 @@
> #define XLNX_VERSAL_NR_UARTS 2
> #define XLNX_VERSAL_NR_GEMS2
> #define XLNX_VERSAL_NR_ADMAS 8
> +#define XLNX_VERSAL_NR_SDS 2
> #define XLNX_VERSAL_NR_IRQS192
>
> typedef struct Versal {
> @@ -58,6 +60,13 @@ typedef struct Versal {
> } iou;
> } lpd;
>
> +/* The Platform Management Controller subsystem. */
> +struct {
> +struct {
> +SDHCIState sd[XLNX_VERSAL_NR_SDS];
> +} iou;
> +} pmc;
> +
> struct {
> MemoryRegion *mr_ddr;
> uint32_t psci_conduit;
> @@ -80,6 +89,7 @@ typedef struct Versal {
> #define VERSAL_GEM1_IRQ_0 58
> #define VERSAL_GEM1_WAKE_IRQ_0 59
> #define VERSAL_ADMA_IRQ_0 60
> +#define VERSAL_SD0_IRQ_0 126
>
> /* Architecturally reserved IRQs suitable for virtualization. */
> #define VERSAL_RSVD_IRQ_FIRST 111
> @@ -129,6 +139,8 @@ typedef struct Versal {
> #define MM_FPD_CRF 0xfd1aU
> #define MM_FPD_CRF_SIZE 0x14
>
> +#define MM_PMC_SD0 0xf104U
> +#define MM_PMC_SD0_SIZE 0x1
> #define MM_PMC_CRP 0xf126U
> #define MM_PMC_CRP_SIZE 0x1
> #endif
>
Re: [PATCH v3 3/6] net/colo-compare.c: Fix deadlock in compare_chr_send
On Wed, 29 Apr 2020 05:37:17 +
"Zhang, Chen" wrote:
> > -Original Message-
> > From: Lukas Straub
> > Sent: Monday, April 27, 2020 3:22 PM
> > To: Zhang, Chen
> > Cc: qemu-devel ; Li Zhijian
> > ; Jason Wang ; Marc-
> > André Lureau ; Paolo Bonzini
> >
> > Subject: Re: [PATCH v3 3/6] net/colo-compare.c: Fix deadlock in
> > compare_chr_send
> >
> > On Mon, 27 Apr 2020 03:36:57 +
> > "Zhang, Chen" wrote:
> >
> > > > -Original Message-
> > > > From: Lukas Straub
> > > > Sent: Monday, April 27, 2020 5:19 AM
> > > > To: qemu-devel
> > > > Cc: Zhang, Chen ; Li Zhijian
> > > > ; Jason Wang ; Marc-
> > > > André Lureau ; Paolo Bonzini
> > > >
> > > > Subject: [PATCH v3 3/6] net/colo-compare.c: Fix deadlock in
> > > > compare_chr_send
> > > >
> > > > The chr_out chardev is connected to a filter-redirector running in
> > > > the main loop. qemu_chr_fe_write_all might block here in
> > > > compare_chr_send if the (socket-)buffer is full.
> > > > If another filter-redirector in the main loop want's to send data to
> > > > chr_pri_in it might also block if the buffer is full. This leads to
> > > > a deadlock because both event loops get blocked.
> > > >
> > > > Fix this by converting compare_chr_send to a coroutine and putting
> > > > the packets in a send queue. Also create a new function
> > > > notify_chr_send, since that should be independend.
> > > >
> > > > Signed-off-by: Lukas Straub
> > > > ---
> > > > net/colo-compare.c | 173 ++---
> >
> > > >
> > > > 1 file changed, 130 insertions(+), 43 deletions(-)
> > > >
> > > > diff --git a/net/colo-compare.c b/net/colo-compare.c index
> > > > 1de4220fe2..ff6a740284 100644
> > > > --- a/net/colo-compare.c
> > > > +++ b/net/colo-compare.c
> > > > @@ -32,6 +32,9 @@
> > > > #include "migration/migration.h"
> > > > #include "util.h"
> > > >
> > > > +#include "block/aio-wait.h"
> > > > +#include "qemu/coroutine.h"
> > > > +
> > > > #define TYPE_COLO_COMPARE "colo-compare"
> > > > #define COLO_COMPARE(obj) \
> > > > OBJECT_CHECK(CompareState, (obj), TYPE_COLO_COMPARE) @@ -
> > 77,6
> > > > +80,20 @@ static int event_unhandled_count;
> > > > *|packet | |packet +|packet | |packet +
> > > > *++ ++++ ++
> > > > */
> > > > +
> > > > +typedef struct SendCo {
> > > > +Coroutine *co;
> > > > +GQueue send_list;
> > > > +bool done;
> > > > +int ret;
> > > > +} SendCo;
> > > > +
> > > > +typedef struct SendEntry {
> > > > +uint32_t size;
> > > > +uint32_t vnet_hdr_len;
> > > > +uint8_t buf[];
> > > > +} SendEntry;
> > > > +
> > > > typedef struct CompareState {
> > > > Object parent;
> > > >
> > > > @@ -91,6 +108,7 @@ typedef struct CompareState {
> > > > SocketReadState pri_rs;
> > > > SocketReadState sec_rs;
> > > > SocketReadState notify_rs;
> > > > +SendCo sendco;
> > > > bool vnet_hdr;
> > > > uint32_t compare_timeout;
> > > > uint32_t expired_scan_cycle;
> > > > @@ -126,8 +144,11 @@ enum {
> > > > static int compare_chr_send(CompareState *s,
> > > > const uint8_t *buf,
> > > > uint32_t size,
> > > > -uint32_t vnet_hdr_len,
> > > > -bool notify_remote_frame);
> > > > +uint32_t vnet_hdr_len);
> > > > +
> > > > +static int notify_chr_send(CompareState *s,
> > > > + const uint8_t *buf,
> > > > + uint32_t size);
> > > >
> > > > static bool packet_matches_str(const char *str,
> > > > const uint8_t *buf, @@ -145,7 +166,7
> > > > @@ static void notify_remote_frame(CompareState *s)
> > > > char msg[] = "DO_CHECKPOINT";
> > > > int ret = 0;
> > > >
> > > > -ret = compare_chr_send(s, (uint8_t *)msg, strlen(msg), 0, true);
> > > > +ret = notify_chr_send(s, (uint8_t *)msg, strlen(msg));
> > > > if (ret < 0) {
> > > > error_report("Notify Xen COLO-frame failed");
> > > > }
> > > > @@ -271,8 +292,7 @@ static void
> > > > colo_release_primary_pkt(CompareState
> > > > *s, Packet *pkt)
> > > > ret = compare_chr_send(s,
> > > > pkt->data,
> > > > pkt->size,
> > > > - pkt->vnet_hdr_len,
> > > > - false);
> > > > + pkt->vnet_hdr_len);
> > > > if (ret < 0) {
> > > > error_report("colo send primary packet failed");
> > > > }
> > > > @@ -699,63 +719,123 @@ static void colo_compare_connection(void
> > > > *opaque, void *user_data)
> > > > }
> > > > }
> > > >
> > > > -static int compare_chr_send(CompareState *s,
> > > > -const uint8_t *buf,
> > > > -uint32_t size,
> > > > -
Re: [PATCH V2] Add a new PIIX option to control PCI hot unplugging of devices on non-root buses
On Wed, Apr 29, 2020 at 07:02:56AM +, Ani Sinha wrote: > > > > On Apr 29, 2020, at 12:27 PM, Michael S. Tsirkin wrote: > > > > On Wed, Apr 29, 2020 at 06:54:52AM +, Ani Sinha wrote: > >> > >> > >>> On Apr 29, 2020, at 12:22 PM, Michael S. Tsirkin wrote: > >>> > >>> On Wed, Apr 29, 2020 at 06:11:20AM +, Ani Sinha wrote: > > > > On Apr 29, 2020, at 10:58 AM, Michael S. Tsirkin > > wrote: > > > > o if there's a need to disable > > just one of these, commit log needs to do a better job documenting the > > usecase. > > The use case is simple. With this feature admins will be able to do what > they were forced to do from Windows driver level but now at the bus > level. Hence, > (a) They need not have access to the guest VM to change or update > windows driver registry settings. They can enable the same setting from > admin management console without any access to VM. > (b) It is more robust. No need to mess with driver settings. Incorrect > settings can brick guest OS. Also no guest specific knowhow required. > (c) It is more scalable since a single cluster wide setting can be used > for all VM power ons and the management plane can take care of the rest > automatically. No need to access individual VMs to enforce this. > (d) I am told that the driver level solution does not persist across a > reboot. > > Ani > >>> > >>> Looks like disabling both plug and unplug would also address these needs. > >> > >> No the driver level solution does not prevent hot plugging of devices but > >> blocks just hot unplugging. The solution I am proposing tries to do the > >> same but from the bus/hypervisor level. > > > > Why does the driver level solution need to prevent just hot unplugging? > > Because it not fair to prevent end users from hot plugging new devices when > it is the (accidental?) hot unplugging of existing devices which causes > issues. Accidental? So maybe what you need is actually something else then - avoid *removing* the device when it's powered down. > > > > > >> > >>> -- > >>> MST
[PATCH v3] audio/jack: add JACK client audiodev
This commit adds a new audiodev backend to allow QEMU to use JACK as
both an audio sink and source.
Signed-off-by: Geoffrey McRae
---
audio/Makefile.objs| 5 +
audio/audio.c | 1 +
audio/audio_template.h | 2 +
audio/jackaudio.c | 615 +
configure | 17 ++
qapi/audio.json| 50 +++-
6 files changed, 688 insertions(+), 2 deletions(-)
create mode 100644 audio/jackaudio.c
diff --git a/audio/Makefile.objs b/audio/Makefile.objs
index d7490a379f..b4a4c11f31 100644
--- a/audio/Makefile.objs
+++ b/audio/Makefile.objs
@@ -28,3 +28,8 @@ common-obj-$(CONFIG_AUDIO_SDL) += sdl.mo
sdl.mo-objs = sdlaudio.o
sdl.mo-cflags := $(SDL_CFLAGS)
sdl.mo-libs := $(SDL_LIBS)
+
+# jack module
+common-obj-$(CONFIG_AUDIO_JACK) += jack.mo
+jack.mo-objs = jackaudio.o
+jack.mo-libs := $(JACK_LIBS)
diff --git a/audio/audio.c b/audio/audio.c
index 7a9e680355..95d9fb16ca 100644
--- a/audio/audio.c
+++ b/audio/audio.c
@@ -1969,6 +1969,7 @@ void audio_create_pdos(Audiodev *dev)
CASE(ALSA, alsa, Alsa);
CASE(COREAUDIO, coreaudio, Coreaudio);
CASE(DSOUND, dsound, );
+CASE(JACK, jack, Jack);
CASE(OSS, oss, Oss);
CASE(PA, pa, Pa);
CASE(SDL, sdl, );
diff --git a/audio/audio_template.h b/audio/audio_template.h
index 7013d3041f..8dd48ce14e 100644
--- a/audio/audio_template.h
+++ b/audio/audio_template.h
@@ -330,6 +330,8 @@ AudiodevPerDirectionOptions *glue(audio_get_pdo_,
TYPE)(Audiodev *dev)
dev->u.coreaudio.TYPE);
case AUDIODEV_DRIVER_DSOUND:
return dev->u.dsound.TYPE;
+case AUDIODEV_DRIVER_JACK:
+return qapi_AudiodevJackPerDirectionOptions_base(dev->u.jack.TYPE);
case AUDIODEV_DRIVER_OSS:
return qapi_AudiodevOssPerDirectionOptions_base(dev->u.oss.TYPE);
case AUDIODEV_DRIVER_PA:
diff --git a/audio/jackaudio.c b/audio/jackaudio.c
new file mode 100644
index 00..a93d361ac4
--- /dev/null
+++ b/audio/jackaudio.c
@@ -0,0 +1,615 @@
+/*
+ * QEMU JACK Audio Connection Kit Client
+ *
+ * Copyright (c) 2020 Geoffrey McRae (gnif)
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a copy
+ * of this software and associated documentation files (the "Software"), to
deal
+ * in the Software without restriction, including without limitation the rights
+ * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
+ * copies of the Software, and to permit persons to whom the Software is
+ * furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice shall be included in
+ * all copies or substantial portions of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
+ * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
FROM,
+ * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
+ * THE SOFTWARE.
+ */
+
+#include "qemu/osdep.h"
+#include "qemu/module.h"
+#include "qemu/fifo8.h"
+#include "qemu-common.h"
+#include "audio.h"
+
+#define AUDIO_CAP "jack"
+#include "audio_int.h"
+
+#include
+#include
+#include
+
+struct QJack;
+
+typedef enum QJackState {
+ QJACK_STATE_DISCONNECTED,
+ QJACK_STATE_CONNECTED,
+ QJACK_STATE_IDLE,
+ QJACK_STATE_RUNNING,
+ QJACK_STATE_STOPPING,
+ QJACK_STATE_STOPPED,
+}
+QJackState;
+
+typedef struct QJackBuffer {
+ int channels;
+ int frames;
+ _Atomic(int) used;
+ int rptr, wptr;
+ float **data;
+}
+QJackBuffer;
+
+typedef struct QJackClient {
+ boolout;
+ QJackState state;
+ jack_client_t *client;
+ jack_nframes_t freq;
+
+ struct QJack *j;
+ int nchannels;
+ int buffersize;
+ jack_port_t **port;
+ QJackBuffer fifo;
+}
+QJackClient;
+
+typedef struct QJackOut {
+HWVoiceOut hw;
+QJackClient c;
+}
+QJackOut;
+
+typedef struct QJackIn {
+HWVoiceIn hw;
+QJackClient c;
+}
+QJackIn;
+
+static void qjack_buffer_create(QJackBuffer *buffer, int channels, int frames)
+{
+buffer->channels = channels;
+buffer->frames = frames;
+buffer->used = 0;
+buffer->rptr = 0;
+buffer->wptr = 0;
+buffer->data = g_malloc(channels * sizeof(float *));
+for (int i = 0; i < channels; ++i) {
+buffer->data[i] = g_malloc(frames * sizeof(float));
+}
+}
+
+static void qjack_buffer_clear(QJackBuffer *buffer)
+{
+atomic_store_explicit(&buffer->used, 0, memory_order_relaxed);
+buffer->rptr = 0;
+buffer->wptr = 0;
+}
+
+static void qjack_buffer_free(QJackBuffer *buffer)
+{
+for (int i = 0; i < buffer->channels; ++i) {
+g_free(buffer->data[i]);
Re: [PATCH] s390x/kvm: help valgrind in several places
+Paolo
On 4/29/20 9:21 AM, Christian Borntraeger wrote:
On 29.04.20 09:00, Philippe Mathieu-Daudé wrote:
Hi Christian,
On 4/28/20 8:31 PM, Christian Borntraeger wrote:
We need some little help in the code to reduce the valgrind noise.
- some designated initializers for the cpu model features and subfunctions
^ This could go as trivial patch while we discuss the rest.
I can certainly split.
If you split then please directly include "Reviewed-by: Philippe
Mathieu-Daudé " to the it.
- mark memory as defined for sida memory reads
Signed-off-by: Christian Borntraeger
---
I couldn't apply this patch, then figured out it targets s390-next.
target/s390x/kvm.c | 15 +--
1 file changed, 13 insertions(+), 2 deletions(-)
diff --git a/target/s390x/kvm.c b/target/s390x/kvm.c
index 69881a0da0..bcd0ee0d14 100644
--- a/target/s390x/kvm.c
+++ b/target/s390x/kvm.c
@@ -52,6 +52,10 @@
#include "hw/s390x/s390-virtio-hcall.h"
#include "hw/s390x/pv.h"
+#ifdef CONFIG_VALGRIND_H
+#include
+#endif
+
#ifndef DEBUG_KVM
#define DEBUG_KVM 0
#endif
@@ -875,6 +879,13 @@ int kvm_s390_mem_op_pv(S390CPU *cpu, uint64_t offset, void
*hostbuf,
error_report("KVM_S390_MEM_OP failed: %s", strerror(-ret));
abort();
}
What about kvm_s390_mem_op()?
I have not triggered something in here, but you are right, there should be
cases where we make conditions
depend of that content. Will change my testing and add something here as well.
+
+#ifdef CONFIG_VALGRIND_H
+ if (!is_write) {
+ VALGRIND_MAKE_MEM_DEFINED(hostbuf, len);
+ }
+#endif
I agree with this macro usage, but think it should be widely accessible by the
whole codebase (and other targets).
"exec/memory.h" is for MemoryRegion and AddressSpace. Maybe "exec/ram_addr.h"
is a better place for common helpers.
If Valgrind is only confused under KVM, the "sysemu/kvm.h" is the obvious place.
This macro IS available for the whole codebase if you include
valgrind/memcheck.h.
We used it in the past (before 2.2) for kvm memory.
See commit 541be9274e8ef227fb1b50ce124fd2cc2dce81a5 (kvm/valgrind: don't mark
memory as initialized).
The only thing that we could discuss is introducing a new global function like
valgrind_make_mem_defined that would hide the ifdefs.
Is there interest in such a thing?
It is likely that these corner cases (valgrind not able to see that this is
defined) are more likely
o happen with KVM. But it would be useful for anything not only KVM.
Correct. What I wanted to say here is, if we can use this code
elsewhere, then it is worth adding a global helper (generic or KVM).
If you reuse this code twice, having an inlined function makes the code
more readable anyway.
See for example in util/coroutine-ucontext.c:
static inline void valgrind_stack_deregister(CoroutineUContext *co)
{
VALGRIND_STACK_DEREGISTER(co->valgrind_stack_id);
}
Maybe we could have something like:
static inline void valgrind_define_memory(void *ptr,
size_t size,
bool is_defined)
{
if (is_defined) {
VALGRIND_MAKE_MEM_DEFINED(ptr, size);
}
}
+
return ret;
}
@@ -2165,7 +2176,7 @@ int kvm_arch_msi_data_to_gsi(uint32_t data)
static int query_cpu_subfunc(S390FeatBitmap features)
{
- struct kvm_s390_vm_cpu_subfunc prop;
+ struct kvm_s390_vm_cpu_subfunc prop = {};
struct kvm_device_attr attr = {
.group = KVM_S390_VM_CPU_MODEL,
.attr = KVM_S390_VM_CPU_MACHINE_SUBFUNC,
@@ -2292,7 +2303,7 @@ static int kvm_to_feat[][2] = {
static int query_cpu_feat(S390FeatBitmap features)
{
- struct kvm_s390_vm_cpu_feat prop;
+ struct kvm_s390_vm_cpu_feat prop = {};
struct kvm_device_attr attr = {
.group = KVM_S390_VM_CPU_MODEL,
.attr = KVM_S390_VM_CPU_MACHINE_FEAT,
Re: [Virtio-fs] [PATCH] virtiofsd: Show submounts
On Tue, Apr 28, 2020 at 9:15 PM Dr. David Alan Gilbert
wrote:
> So our current sequence is:
>
>(new namespace)
> 1)if (mount(NULL, "/", NULL, MS_REC | MS_SLAVE, NULL) < 0) {
> 2) if (mount("proc", "/proc", "proc",
>
> 3) if (mount(source, source, NULL, MS_BIND | MS_REC, NULL) < 0) {
> 4) (chdir newroot, pivot, chdir oldroot)
> 5) if (mount("", ".", "", MS_SLAVE | MS_REC, NULL) < 0) {
> 6) if (umount2(".", MNT_DETACH) < 0) {
>
> So are you saying we need a:
>if (mount(NULL, "/", NULL, MS_REC | MS_SHARED, NULL) < 0) {
>
> and can this go straight after (1) ?
Or right before (3). Important thing is that that new mount will
only receive propagation if the type of the mount at source (before
(3) is performed) is shared.
Thanks,
Miklos
Re: [PATCH 16/17] spapr_pci: Drop some dead error handling
On 4/28/20 6:34 PM, Markus Armbruster wrote:
chassis_from_bus() uses object_property_get_uint() to get property
"chassis_nr" of the bridge device. Failure would be a programming
error. Pass &error_abort, and simplify its callers.
Cc: David Gibson
Cc: qemu-...@nongnu.org
Signed-off-by: Markus Armbruster
---
hw/ppc/spapr_pci.c | 86 ++
1 file changed, 18 insertions(+), 68 deletions(-)
diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
index 1d73d05a0a..b6036be51c 100644
--- a/hw/ppc/spapr_pci.c
+++ b/hw/ppc/spapr_pci.c
@@ -1203,46 +1203,36 @@ static SpaprDrc *drc_from_devfn(SpaprPhbState *phb,
drc_id_from_devfn(phb, chassis, devfn));
}
-static uint8_t chassis_from_bus(PCIBus *bus, Error **errp)
+static uint8_t chassis_from_bus(PCIBus *bus)
{
if (pci_bus_is_root(bus)) {
return 0;
} else {
PCIDevice *bridge = pci_bridge_get_device(bus);
-return object_property_get_uint(OBJECT(bridge), "chassis_nr", errp);
+return object_property_get_uint(OBJECT(bridge), "chassis_nr",
+&error_abort);
}
}
static SpaprDrc *drc_from_dev(SpaprPhbState *phb, PCIDevice *dev)
{
-Error *local_err = NULL;
-uint8_t chassis = chassis_from_bus(pci_get_bus(dev), &local_err);
-
-if (local_err) {
-error_report_err(local_err);
-return NULL;
-}
+uint8_t chassis = chassis_from_bus(pci_get_bus(dev));
return drc_from_devfn(phb, chassis, dev->devfn);
}
-static void add_drcs(SpaprPhbState *phb, PCIBus *bus, Error **errp)
+static void add_drcs(SpaprPhbState *phb, PCIBus *bus)
{
Object *owner;
int i;
uint8_t chassis;
-Error *local_err = NULL;
if (!phb->dr_enabled) {
return;
}
-chassis = chassis_from_bus(bus, &local_err);
-if (local_err) {
-error_propagate(errp, local_err);
-return;
-}
+chassis = chassis_from_bus(bus);
if (pci_bus_is_root(bus)) {
owner = OBJECT(phb);
@@ -1256,21 +1246,16 @@ static void add_drcs(SpaprPhbState *phb, PCIBus *bus,
Error **errp)
}
}
-static void remove_drcs(SpaprPhbState *phb, PCIBus *bus, Error **errp)
+static void remove_drcs(SpaprPhbState *phb, PCIBus *bus)
{
int i;
uint8_t chassis;
-Error *local_err = NULL;
if (!phb->dr_enabled) {
return;
}
-chassis = chassis_from_bus(bus, &local_err);
-if (local_err) {
-error_propagate(errp, local_err);
-return;
-}
+chassis = chassis_from_bus(bus);
for (i = PCI_SLOT_MAX * PCI_FUNC_MAX - 1; i >= 0; i--) {
SpaprDrc *drc = drc_from_devfn(phb, chassis, i);
@@ -1488,17 +1473,11 @@ int spapr_pci_dt_populate(SpaprDrc *drc,
SpaprMachineState *spapr,
}
static void spapr_pci_bridge_plug(SpaprPhbState *phb,
- PCIBridge *bridge,
- Error **errp)
+ PCIBridge *bridge)
{
-Error *local_err = NULL;
PCIBus *bus = pci_bridge_get_sec_bus(bridge);
-add_drcs(phb, bus, &local_err);
-if (local_err) {
-error_propagate(errp, local_err);
-return;
-}
+add_drcs(phb, bus);
}
static void spapr_pci_plug(HotplugHandler *plug_handler,
@@ -1529,11 +1508,7 @@ static void spapr_pci_plug(HotplugHandler *plug_handler,
g_assert(drc);
if (pc->is_bridge) {
-spapr_pci_bridge_plug(phb, PCI_BRIDGE(plugged_dev), &local_err);
-if (local_err) {
-error_propagate(errp, local_err);
-return;
-}
+spapr_pci_bridge_plug(phb, PCI_BRIDGE(plugged_dev));
}
/* Following the QEMU convention used for PCIe multifunction
@@ -1560,12 +1535,7 @@ static void spapr_pci_plug(HotplugHandler *plug_handler,
spapr_drc_reset(drc);
} else if (PCI_FUNC(pdev->devfn) == 0) {
int i;
-uint8_t chassis = chassis_from_bus(pci_get_bus(pdev), &local_err);
-
-if (local_err) {
-error_propagate(errp, local_err);
-return;
-}
+uint8_t chassis = chassis_from_bus(pci_get_bus(pdev));
for (i = 0; i < 8; i++) {
SpaprDrc *func_drc;
@@ -1587,17 +1557,11 @@ out:
}
static void spapr_pci_bridge_unplug(SpaprPhbState *phb,
-PCIBridge *bridge,
-Error **errp)
+PCIBridge *bridge)
{
-Error *local_err = NULL;
PCIBus *bus = pci_bridge_get_sec_bus(bridge);
-remove_drcs(phb, bus, &local_err);
-if (local_err) {
-error_propagate(errp, local_err);
-return;
-}
+remove_drcs(phb, bus);
}
static void spapr_pci_unplug(HotplugHandler *plug_handler,
@@ -1619,11 +1583,7 @@ static void spapr_pci_unplug(HotplugHandler
Re: [PATCH 4/4] block: Use blk_make_empty() after commits
On 28.04.20 16:07, Eric Blake wrote:
> On 4/28/20 8:26 AM, Max Reitz wrote:
>> bdrv_commit() already has a BlockBackend pointing to the BDS that we
>> want to empty, it just has the wrong permissions.
>>
>> qemu-img commit has no BlockBackend pointing to the old backing file
>> yet, but introducing one is simple.
>>
>> After this commit, bdrv_make_empty() is the only remaining caller of
>> BlockDriver.bdrv_make_empty().
>>
>> Signed-off-by: Max Reitz
>> ---
>> block/commit.c | 8 +++-
>> qemu-img.c | 19 ++-
>> 2 files changed, 21 insertions(+), 6 deletions(-)
>>
>> diff --git a/block/commit.c b/block/commit.c
>> index 8e672799af..24720ba67d 100644
>> --- a/block/commit.c
>> +++ b/block/commit.c
>> @@ -493,10 +493,16 @@ int bdrv_commit(BlockDriverState *bs)
>> }
>> if (drv->bdrv_make_empty) {
>
> This 'if' is still a bit awkward. Do all filter drivers set this
> function, or will bdrv_make_empty() automatically take care of looking
> through filters? Should this be a check of a supported_ variable
> instead (similar to how Kevin just added supported_truncate_flags)?
>
>> - ret = drv->bdrv_make_empty(bs);
>> + ret = blk_set_perm(src, BLK_PERM_WRITE, BLK_PERM_ALL, NULL);
>> if (ret < 0) {
>> goto ro_cleanup;
>> }
>> +
>> + ret = blk_make_empty(src, NULL);
>
> So, if the driver lacks the callback, you miss calling blk_make_empty()
> even if it would have done something.
We can’t just call blk_make_empty() and then special case -ENOTSUP here,
though, because the BB doesn’t have a WRITE permission beforehand. So
we have to take that permission before we can call blk_make_empty().
But taking the permission can fail, and then we kind of have to report
the -EPERM, even though the BlockDriver may not support emptying anyway.
I suppose if we just have to take the WRITE_UNCHANGED permission,
though, we can assume that that’s basically always allowed, so it
shouldn’t be that much of a problem there.
Max
>> + if (ret < 0) {
>> + goto ro_cleanup;
>> + }
>> +
>> blk_flush(src);
>> }
>> diff --git a/qemu-img.c b/qemu-img.c
>> index 821cbf610e..a5e8659867 100644
>> --- a/qemu-img.c
>> +++ b/qemu-img.c
>> @@ -1065,11 +1065,20 @@ static int img_commit(int argc, char **argv)
>> goto unref_backing;
>> }
>> - if (!drop && bs->drv->bdrv_make_empty) {
>> - ret = bs->drv->bdrv_make_empty(bs);
>
> Old code: if the driver had the callback, use it.
>
>> - if (ret) {
>> - error_setg_errno(&local_err, -ret, "Could not empty %s",
>> - filename);
>> + if (!drop) {
>> + BlockBackend *old_backing_blk;
>> +
>> + old_backing_blk = blk_new_with_bs(bs, BLK_PERM_WRITE,
>> BLK_PERM_ALL,
>> + &local_err);
>> + if (!old_backing_blk) {
>> + goto unref_backing;
>> + }
>> + ret = blk_make_empty(old_backing_blk, &local_err);
>
> New code: Call blk_make_empty() whether or not driver has the callback,
> then deal with the fallout.
>
>> + blk_unref(old_backing_blk);
>> + if (ret == -ENOTSUP) {
>> + error_free(local_err);
>> + local_err = NULL;
>> + } else if (ret < 0) {
>> goto unref_backing;
>> }
>
> But this actually looks smarter than the commit case.
>
signature.asc
Description: OpenPGP digital signature
Re: [PATCH 4/4] block: Use blk_make_empty() after commits
On 28.04.20 17:03, Kevin Wolf wrote:
> Am 28.04.2020 um 15:26 hat Max Reitz geschrieben:
>> bdrv_commit() already has a BlockBackend pointing to the BDS that we
>> want to empty, it just has the wrong permissions.
>>
>> qemu-img commit has no BlockBackend pointing to the old backing file
>> yet, but introducing one is simple.
>>
>> After this commit, bdrv_make_empty() is the only remaining caller of
>> BlockDriver.bdrv_make_empty().
>>
>> Signed-off-by: Max Reitz
>> ---
>> block/commit.c | 8 +++-
>> qemu-img.c | 19 ++-
>> 2 files changed, 21 insertions(+), 6 deletions(-)
>>
>> diff --git a/block/commit.c b/block/commit.c
>> index 8e672799af..24720ba67d 100644
>> --- a/block/commit.c
>> +++ b/block/commit.c
>> @@ -493,10 +493,16 @@ int bdrv_commit(BlockDriverState *bs)
>> }
>>
>> if (drv->bdrv_make_empty) {
>> -ret = drv->bdrv_make_empty(bs);
>> +ret = blk_set_perm(src, BLK_PERM_WRITE, BLK_PERM_ALL, NULL);
>
> This is very likely to fail because the common case is that the source
> node is attached to a guest device that doesn't share writes.
> (qemu-iotests 131 and 274 catch this.)
>
> So I think after my theoretical comment in patch 1, this is the
> practical reason why we need WRITE_UNCHANGED rather than WRITE.
>
> Also, why don't you take this permission from the start so that we would
> error out right away rather than failing after waiting for the all the
> data to be copied?
Because we only need to take it when the BlockDriver actually supports
bdrv_make_empty(), so I thought I’d put it here where we have the check
anyway.
However, yes, when we take WRITE_UNCHANGED, we might as well take it
unconditionally from the start. (And then call blk_make_empty()
unconditionally here, too, and let it figure out -ENOTSUP, like Eric noted.)
>> if (ret < 0) {
>> goto ro_cleanup;
>> }
>> +
>> +ret = blk_make_empty(src, NULL);
>> +if (ret < 0) {
>> +goto ro_cleanup;
>> +}
>> +
>> blk_flush(src);
>> }
>>
>> diff --git a/qemu-img.c b/qemu-img.c
>> index 821cbf610e..a5e8659867 100644
>> --- a/qemu-img.c
>> +++ b/qemu-img.c
>> @@ -1065,11 +1065,20 @@ static int img_commit(int argc, char **argv)
>> goto unref_backing;
>> }
>>
>> -if (!drop && bs->drv->bdrv_make_empty) {
>> -ret = bs->drv->bdrv_make_empty(bs);
>> -if (ret) {
>> -error_setg_errno(&local_err, -ret, "Could not empty %s",
>> - filename);
>> +if (!drop) {
>> +BlockBackend *old_backing_blk;
>> +
>> +old_backing_blk = blk_new_with_bs(bs, BLK_PERM_WRITE, BLK_PERM_ALL,
>> + &local_err);
>
> Oh, you actually depend on another series that you didn't mention in
> the cover letter.
Well, yes. I didn’t really realize because I just based it on my
block-next...
Max
signature.asc
Description: OpenPGP digital signature
Re: [PATCH 3/4] block: Add blk_make_empty()
On 28.04.20 16:47, Kevin Wolf wrote:
> Am 28.04.2020 um 15:26 hat Max Reitz geschrieben:
>> Two callers of BlockDriver.bdrv_make_empty() remain that should not call
>> this method directly. Both do not have access to a BdrvChild, but they
>> can use a BlockBackend, so we add this function that lets them use it.
>>
>> Signed-off-by: Max Reitz
>> ---
>> include/sysemu/block-backend.h | 2 ++
>> block/block-backend.c | 5 +
>> 2 files changed, 7 insertions(+)
>>
>> diff --git a/include/sysemu/block-backend.h b/include/sysemu/block-backend.h
>> index d37c1244dd..14338b76dc 100644
>> --- a/include/sysemu/block-backend.h
>> +++ b/include/sysemu/block-backend.h
>> @@ -266,4 +266,6 @@ int coroutine_fn blk_co_copy_range(BlockBackend *blk_in,
>> int64_t off_in,
>>
>> const BdrvChild *blk_root(BlockBackend *blk);
>>
>> +int blk_make_empty(BlockBackend *blk, Error **errp);
>> +
>> #endif
>> diff --git a/block/block-backend.c b/block/block-backend.c
>> index 3592066b42..5d36efd32f 100644
>> --- a/block/block-backend.c
>> +++ b/block/block-backend.c
>> @@ -2402,3 +2402,8 @@ const BdrvChild *blk_root(BlockBackend *blk)
>> {
>> return blk->root;
>> }
>> +
>> +int blk_make_empty(BlockBackend *blk, Error **errp)
>> +{
>> +return bdrv_make_empty(blk->root, errp);
>> +}
>
> Should we check that blk->root != NULL? Most other functions do that
> through blk_is_available().
Why not.
Max
signature.asc
Description: OpenPGP digital signature
Re: [RFC PATCH] qom: Implement qom-get HMP command
On 4/27/20 9:19 PM, Dr. David Alan Gilbert wrote: > * Cédric Le Goater (c...@kaod.org) wrote: >> From: "Dr. David Alan Gilbert" >> >> Reimplement it based on qmp_qom_get() to avoid converting QObjects back >> to strings. > > > I'd love to see this or something similar in; what does it's output > look like for structures - I think that was the main problem people > complained about last time, although IMHO even a version that couldn't > do structures nicely would be better than nothing. Should we merge this patch then ? C.
Re: [PATCH v1 11/11] hw/arm: versal-virt: Add support for the RTC
On 4/27/20 8:16 PM, Edgar E. Iglesias wrote:
> From: "Edgar E. Iglesias"
>
> Add support for the RTC.
>
> Signed-off-by: Edgar E. Iglesias
Reviewed-by: Luc Michel
> ---
> hw/arm/xlnx-versal-virt.c | 22 ++
> 1 file changed, 22 insertions(+)
>
> diff --git a/hw/arm/xlnx-versal-virt.c b/hw/arm/xlnx-versal-virt.c
> index 0afee48672..7e749e1926 100644
> --- a/hw/arm/xlnx-versal-virt.c
> +++ b/hw/arm/xlnx-versal-virt.c
> @@ -283,6 +283,27 @@ static void fdt_add_sd_nodes(VersalVirt *s)
> }
> }
>
> +static void fdt_add_rtc_node(VersalVirt *s)
> +{
> +const char compat[] = "xlnx,zynqmp-rtc";
> +const char interrupt_names[] = "alarm\0sec";
> +char *name = g_strdup_printf("/rtc@%x", MM_PMC_RTC);
> +
> +qemu_fdt_add_subnode(s->fdt, name);
> +
> +qemu_fdt_setprop_cells(s->fdt, name, "interrupts",
> + GIC_FDT_IRQ_TYPE_SPI, VERSAL_RTC_ALARM_IRQ,
> + GIC_FDT_IRQ_FLAGS_LEVEL_HI,
> + GIC_FDT_IRQ_TYPE_SPI, VERSAL_RTC_SECONDS_IRQ,
> + GIC_FDT_IRQ_FLAGS_LEVEL_HI);
> +qemu_fdt_setprop(s->fdt, name, "interrupt-names",
> + interrupt_names, sizeof(interrupt_names));
> +qemu_fdt_setprop_sized_cells(s->fdt, name, "reg",
> + 2, MM_PMC_RTC, 2, MM_PMC_RTC_SIZE);
> +qemu_fdt_setprop(s->fdt, name, "compatible", compat, sizeof(compat));
> +g_free(name);
> +}
> +
> static void fdt_nop_memory_nodes(void *fdt, Error **errp)
> {
> Error *err = NULL;
> @@ -496,6 +517,7 @@ static void versal_virt_init(MachineState *machine)
> fdt_add_timer_nodes(s);
> fdt_add_zdma_nodes(s);
> fdt_add_sd_nodes(s);
> +fdt_add_rtc_node(s);
> fdt_add_cpu_nodes(s, psci_conduit);
> fdt_add_clk_node(s, "/clk125", 12500, s->phandle.clk_125Mhz);
> fdt_add_clk_node(s, "/clk25", 2500, s->phandle.clk_25Mhz);
>
Re: [PATCH V2] Add a new PIIX option to control PCI hot unplugging of devices on non-root buses
On Wed, Apr 29, 2020 at 07:43:04AM +, Ani Sinha wrote: > > > > On Apr 29, 2020, at 1:08 PM, Michael S. Tsirkin wrote: > > > > On Wed, Apr 29, 2020 at 07:02:56AM +, Ani Sinha wrote: > >> > >> > >>> On Apr 29, 2020, at 12:27 PM, Michael S. Tsirkin wrote: > >>> > >>> On Wed, Apr 29, 2020 at 06:54:52AM +, Ani Sinha wrote: > > > > On Apr 29, 2020, at 12:22 PM, Michael S. Tsirkin > > wrote: > > > > On Wed, Apr 29, 2020 at 06:11:20AM +, Ani Sinha wrote: > >> > >> > >>> On Apr 29, 2020, at 10:58 AM, Michael S. Tsirkin > >>> wrote: > >>> > >>> o if there's a need to disable > >>> just one of these, commit log needs to do a better job documenting the > >>> usecase. > >> > >> The use case is simple. With this feature admins will be able to do > >> what they were forced to do from Windows driver level but now at the > >> bus level. Hence, > >> (a) They need not have access to the guest VM to change or update > >> windows driver registry settings. They can enable the same setting > >> from admin management console without any access to VM. > >> (b) It is more robust. No need to mess with driver settings. Incorrect > >> settings can brick guest OS. Also no guest specific knowhow required. > >> (c) It is more scalable since a single cluster wide setting can be > >> used for all VM power ons and the management plane can take care of > >> the rest automatically. No need to access individual VMs to enforce > >> this. > >> (d) I am told that the driver level solution does not persist across a > >> reboot. > >> > >> Ani > > > > Looks like disabling both plug and unplug would also address these > > needs. > > No the driver level solution does not prevent hot plugging of devices > but blocks just hot unplugging. The solution I am proposing tries to do > the same but from the bus/hypervisor level. > >>> > >>> Why does the driver level solution need to prevent just hot unplugging? > >> > >> Because it not fair to prevent end users from hot plugging new devices > >> when it is the (accidental?) hot unplugging of existing devices which > >> causes issues. > > > > Accidental? So maybe what you need is actually something else then - > > avoid *removing* the device when it's powered down. > > You don’t get it. It is not hypervisor admins who are unplugging it. It is > the end users. Even RedHat customers want this feature. See following > resources: > https://www.redhat.com/archives/libvir-list/2020-February/msg00110.html > https://bugzilla.redhat.com/show_bug.cgi?id=1802592 > https://bugzilla.redhat.com/show_bug.cgi?id=1790899 That doesn't seem to require that hotplug keeps working. > My approach is much more fine grained than just disable everything approach > that we have for q35. For i440fx we can do better than that. > > > > > >>> > >>> > > > -- > > MST >
Re: [PATCH v2 00/15] qapi: Spring cleaning
Queued.
[Bug 1875819] [NEW] [Feature request] prebuilt testing docker images
Public bug reported: Instead of building qemu:docker images locally, we should pull the one built from Travis/Shippable/GitLab by default, and build it only when manually requested. ** Affects: qemu Importance: Undecided Status: New -- You received this bug notification because you are a member of qemu- devel-ml, which is subscribed to QEMU. https://bugs.launchpad.net/bugs/1875819 Title: [Feature request] prebuilt testing docker images Status in QEMU: New Bug description: Instead of building qemu:docker images locally, we should pull the one built from Travis/Shippable/GitLab by default, and build it only when manually requested. To manage notifications about this bug go to: https://bugs.launchpad.net/qemu/+bug/1875819/+subscriptions
Re: [PATCH v10 00/14] iotests: use python logging
On 28.04.20 19:36, John Snow wrote: > > > On 4/28/20 8:21 AM, Kevin Wolf wrote: >> Am 28.04.2020 um 13:46 hat Max Reitz geschrieben: >>> On 31.03.20 02:00, John Snow wrote: This series uses python logging to enable output conditionally on iotests.log(). We unify an initialization call (which also enables debugging output for those tests with -d) and then make the switch inside of iotests. It will help alleviate the need to create logged/unlogged versions of all the various helpers we have made. Also, I got lost and accidentally delinted iotests while I was here. Sorry about that. By version 9, it's now the overwhelming focus of this series. No good deed, etc. >>> >>> Seems like nobody else wants it, so I thank you and let you know that >>> I’ve applied this series to my block-next branch: >>> >>> https://git.xanclic.moe/XanClic/qemu/commits/branch/block-next >> >> John said he wanted to address my comment on patch 14, so I expected him >> to send another version. This need not stop this series (we can still >> fix that on top), but just as an explanation why I didn't take it yet. >> >> Kevin >> > > Sorry, foggy memory. I will send a follow-up and we can try to squash it in. All right. Max signature.asc Description: OpenPGP digital signature
Re: [PATCH for-5.1 4/7] target/mips: Add Loongson-3 CPU definition
On 4/29/20 5:51 AM, Huacai Chen wrote:
Hi, Aleksandar,
I've tried translate.google.com, and documents are available here:
Loongson-3A R1 (Loongson-3A1000)
User Manual Part 1:
http://ftp.godson.ac.cn/lemote/3A1000_p1.pdf
http://ftp.godson.ac.cn/lemote/Loongson3A1000_processor_user_manual_P1.pdf
(Chinese Version)
User Manual Part 2:
http://ftp.godson.ac.cn/lemote/3A1000_p2.pdf
http://ftp.godson.ac.cn/lemote/Loongson3A1000_processor_user_manual_P2.pdf
(Chinese Version)
Loongson-3A R2 (Loongson-3A2000)
User Manual Part 1:
http://ftp.godson.ac.cn/lemote/3A2000_p1.pdf
http://ftp.godson.ac.cn/lemote/Loongson3A2000_user1.pdf (Chinese Version)
User Manual Part 2:
http://ftp.godson.ac.cn/lemote/3A2000_p2.pdf
http://ftp.godson.ac.cn/lemote/Loongson3A2000_user2.pdf (Chinese Version)
Loongson-3A R3 (Loongson-3A3000)
User Manual Part 1:
http://ftp.godson.ac.cn/lemote/3A3000_p1.pdf
http://ftp.godson.ac.cn/lemote/Loongson3A3000_3B3000usermanual1.pdf
(Chinese Version)
User Manual Part 2:
http://ftp.godson.ac.cn/lemote/3A3000_p2.pdf
http://ftp.godson.ac.cn/lemote/Loongson3A3000_3B3000usermanual2.pdf
(Chinese Version)
Loongson-3A R4 (Loongson-3A4000)
User Manual Part 1:
http://ftp.godson.ac.cn/lemote/3A4000_p1.pdf
http://ftp.godson.ac.cn/lemote/3A4000user.pdf (Chinese Version)
User Manual Part 2:
I'm sorry that it is unavailable now.
Thanks for the translations!
Since we can only review Loongson-3A R3, are there specific features
from R4 you need that are not available in R3?
On Wed, Apr 29, 2020 at 2:37 AM Aleksandar Markovic
wrote:
Huacai,
Can you please do machine translation of the document?
It can be done via translate.google.com (it accepts pdf files, but
does not have download feature, and workaround is to "print to pdf"...
Thanks in advance!
Aleksandar
уто, 28. апр 2020. у 10:26 chen huacai је написао/ла:
Hi, Philippe,
On Tue, Apr 28, 2020 at 2:34 PM Philippe Mathieu-Daudé wrote:
Hi Huacai,
On 4/27/20 11:33 AM, Huacai Chen wrote:
Loongson-3 CPU family include Loongson-3A R1/R2/R3/R4 and Loongson-3B
R1/R2. Loongson-3A R4 is the newest and its ISA is almost the superset
of all others. To reduce complexity, we just define a "Loongson-3A" CPU
which is corresponding to Loongson-3A R4. Loongson-3A has CONFIG6 and
CONFIG7, so add their bit-fields as well.
Is there a public datasheet for R4? (If possible in English).
I'm sorry that we only have Chinese datasheet in www.loongson.cn.
Signed-off-by: Huacai Chen
Co-developed-by: Jiaxun Yang
---
target/mips/cpu.h| 28 ++
target/mips/internal.h | 2 ++
target/mips/mips-defs.h | 7 --
target/mips/translate.c | 2 ++
target/mips/translate_init.inc.c | 51
5 files changed, 88 insertions(+), 2 deletions(-)
diff --git a/target/mips/cpu.h b/target/mips/cpu.h
index 94d01ea..0b3c987 100644
--- a/target/mips/cpu.h
+++ b/target/mips/cpu.h
@@ -940,7 +940,35 @@ struct CPUMIPSState {
#define CP0C5_UFR 2
#define CP0C5_NFExists 0
int32_t CP0_Config6;
+int32_t CP0_Config6_rw_bitmask;
+#define CP0C6_BPPASS 31
+#define CP0C6_KPOS24
+#define CP0C6_KE 23
+#define CP0C6_VTLBONLY22
+#define CP0C6_LASX21
+#define CP0C6_SSEN20
+#define CP0C6_DISDRTIME 19
+#define CP0C6_PIXNUEN 18
+#define CP0C6_SCRAND 17
+#define CP0C6_LLEXCEN 16
+#define CP0C6_DISVC 15
+#define CP0C6_VCLRU 14
+#define CP0C6_DCLRU 13
+#define CP0C6_PIXUEN 12
+#define CP0C6_DISBLKLYEN 11
+#define CP0C6_UMEMUALEN 10
+#define CP0C6_SFBEN 8
+#define CP0C6_FLTINT 7
+#define CP0C6_VLTINT 6
+#define CP0C6_DISBTB 5
+#define CP0C6_STPREFCTL 2
+#define CP0C6_INSTPREF1
+#define CP0C6_DATAPREF0
int32_t CP0_Config7;
+int64_t CP0_Config7_rw_bitmask;
+#define CP0C7_NAPCGEN 2
+#define CP0C7_UNIMUEN 1
+#define CP0C7_VFPUCGEN 0
uint64_t CP0_LLAddr;
uint64_t CP0_MAAR[MIPS_MAAR_MAX];
int32_t CP0_MAARI;
diff --git a/target/mips/internal.h b/target/mips/internal.h
index 1bf274b..7853cb1 100644
--- a/target/mips/internal.h
+++ b/target/mips/internal.h
@@ -36,7 +36,9 @@ struct mips_def_t {
int32_t CP0_Config5;
int32_t CP0_Config5_rw_bitmask;
int32_t CP0_Config6;
+int32_t CP0_Config6_rw_bitmask;
int32_t CP0_Config7;
+int32_t CP0_Config7_rw_bitmask;
target_ulong CP0_LLAddr_rw_bitmask;
int CP0_LLAddr_shift;
int32_t SYNCI_Step;
diff --git a/target/mips/mips-defs.h b/target/mips/mips-defs.h
index a831bb4..c2c96db 100644
--- a/target/mips/mips-defs.h
+++ b/target/mips/mips-defs.h
@@ -51,8 +51,9 @@
*/
#define INSN_LOONGSON2E 0x0001ULL
#define INSN_LOONGSON2F 0x0002ULL
-#define INSN_VR54XX 0x0004ULL
-#define INSN_R59000x000800
RE: [PATCH v2 1/2] Fix undefined behaviour
> -Original Message- > From: Grzegorz Uriasz > Sent: 29 April 2020 04:04 > To: qemu-devel@nongnu.org > Cc: Grzegorz Uriasz ; marma...@invisiblethingslab.com; > ar...@puzio.waw.pl; > ja...@bartmin.ski; j.nowa...@student.uw.edu.pl; Stefano Stabellini > ; Anthony > Perard ; Paul Durrant ; > xen-de...@lists.xenproject.org > Subject: [PATCH v2 1/2] Fix undefined behaviour > > This patch fixes qemu crashes when passing through an IGD device to HVM > guests under XEN. The problem > is that on almost every laptop > reading the IGD ROM from SYSFS will fail, the reason for it is that the IGD > rom is polymorphic and it > modifies itself > during bootup - this results in an invalid rom image - the kernel checks > whether the image is valid > and when that's not the case > it won't allow userspace to read it. It seems that when the code was first > written(xen_pt_load_rom.c) > the kernel's back then didn't check > whether the rom is valid and just passed the contents to userspace - because > of this qemu also tries > to repair the rom later in the code. > When stating the rom the kernel isn't validating the rom contents so it is > returning the proper size > of the rom(32 4kb pages). > > This results in undefined behaviour - pci_assign_dev_load_option_rom is > creating a buffer and then > writes the size of the buffer to a pointer. > In pci_assign_dev_load_option_rom under old kernels if the fstat would > succeed then fread would also > succeed - this means if the buffer > is allocated the size of the buffer will be set. On newer kernels this is not > the case - on all > laptops I've tested(spanning a few > generations of IGD) the fstat is successful and the buffer is allocated but > the fread is failing - as > the buffer is not deallocated > the function is returning a valid pointer without setting the size of the > buffer for the caller. The > caller of pci_assign_dev_load_option_rom > is holding the size of the buffer in an uninitialized variable and is just > checking whether > pci_assign_dev_load_option_rom returned a valid pointer. > This later results in cpu_physical_memory_write(0xc, > result_of_pci_assign_dev_load_option_rom, > unitialized_variable) which > depending on the compiler parameters, configure flags, etc... might crash > qemu or might work - the xen > 4.12 stable release with default configure > parameters works but changing the compiler options slightly(for instance by > enabling qemu debug) > results in qemu crashing ¯\_(;-;)_/¯ > > The only situation when the original pci_assign_dev_load_option_rom works is > when the IDG is not the I think you mean IGD > boot gpu - this won't happen on any laptop and > will be rare on desktops. > > This patch deallocates the buffer and returns NULL if reading the VBIOS fails > - the caller of the > function then properly shuts down qemu. > The next patch in the series introduces a better method for getting the vbios > so qemu does not exit > when pci_assign_dev_load_option_rom fails - > this is the reason I've changed error_report to warn_report as whether a > failure in > pci_assign_dev_load_option_rom is fatal > depends on the caller. > > Signed-off-by: Grzegorz Uriasz With the typo fixed... Reviewed-by: Paul Durrant
Re: [PATCH V2] Add a new PIIX option to control PCI hot unplugging of devices on non-root buses
> On Apr 29, 2020, at 1:39 PM, Michael S. Tsirkin wrote: > > On Wed, Apr 29, 2020 at 07:43:04AM +, Ani Sinha wrote: >> >> >>> On Apr 29, 2020, at 1:08 PM, Michael S. Tsirkin wrote: >>> >>> On Wed, Apr 29, 2020 at 07:02:56AM +, Ani Sinha wrote: > On Apr 29, 2020, at 12:27 PM, Michael S. Tsirkin wrote: > > On Wed, Apr 29, 2020 at 06:54:52AM +, Ani Sinha wrote: >> >> >>> On Apr 29, 2020, at 12:22 PM, Michael S. Tsirkin >>> wrote: >>> >>> On Wed, Apr 29, 2020 at 06:11:20AM +, Ani Sinha wrote: > On Apr 29, 2020, at 10:58 AM, Michael S. Tsirkin > wrote: > > o if there's a need to disable > just one of these, commit log needs to do a better job documenting the > usecase. The use case is simple. With this feature admins will be able to do what they were forced to do from Windows driver level but now at the bus level. Hence, (a) They need not have access to the guest VM to change or update windows driver registry settings. They can enable the same setting from admin management console without any access to VM. (b) It is more robust. No need to mess with driver settings. Incorrect settings can brick guest OS. Also no guest specific knowhow required. (c) It is more scalable since a single cluster wide setting can be used for all VM power ons and the management plane can take care of the rest automatically. No need to access individual VMs to enforce this. (d) I am told that the driver level solution does not persist across a reboot. Ani >>> >>> Looks like disabling both plug and unplug would also address these >>> needs. >> >> No the driver level solution does not prevent hot plugging of devices >> but blocks just hot unplugging. The solution I am proposing tries to do >> the same but from the bus/hypervisor level. > > Why does the driver level solution need to prevent just hot unplugging? Because it not fair to prevent end users from hot plugging new devices when it is the (accidental?) hot unplugging of existing devices which causes issues. >>> >>> Accidental? So maybe what you need is actually something else then - >>> avoid *removing* the device when it's powered down. >> >> You don’t get it. It is not hypervisor admins who are unplugging it. It is >> the end users. Even RedHat customers want this feature. See following >> resources: >> https://urldefense.proofpoint.com/v2/url?u=https-3A__www.redhat.com_archives_libvir-2Dlist_2020-2DFebruary_msg00110.html&d=DwIFaQ&c=s883GpUCOChKOHiocYtGcg&r=IIUxIyRwG4RGy57y2nvMNYcDkqW-NHozZ2R38VYcg5U&m=Mf70_yU9LUbRFZOy4rYM5N43B_MDbO7SxEMSSJKVaJY&s=KgR1-KlzL-bGr51uY1vupOIgTpTjNAecbuOUIpcuMUs&e= >> >> https://urldefense.proofpoint.com/v2/url?u=https-3A__bugzilla.redhat.com_show-5Fbug.cgi-3Fid-3D1802592&d=DwIFaQ&c=s883GpUCOChKOHiocYtGcg&r=IIUxIyRwG4RGy57y2nvMNYcDkqW-NHozZ2R38VYcg5U&m=Mf70_yU9LUbRFZOy4rYM5N43B_MDbO7SxEMSSJKVaJY&s=KVis9gzVeA7nnGauZpXWm_sEnl_UpsIzSlggwb60Fg8&e= >> >> https://urldefense.proofpoint.com/v2/url?u=https-3A__bugzilla.redhat.com_show-5Fbug.cgi-3Fid-3D1790899&d=DwIFaQ&c=s883GpUCOChKOHiocYtGcg&r=IIUxIyRwG4RGy57y2nvMNYcDkqW-NHozZ2R38VYcg5U&m=Mf70_yU9LUbRFZOy4rYM5N43B_MDbO7SxEMSSJKVaJY&s=Y0K8tiyqpmKeXU245pnhTTAr2e3YSuxxw4BkiDxGwB8&e= >> > > That doesn't seem to require that hotplug keeps working. Like I said, that is because PCIE limits this at this moment. We can do better than this solution on i440fx. We can be narrower in limiting just hot unplug leaving hot plugging as is. Why should we always go with the least common denominator for all the features? Why do we have to be limited when we can do better? > >> My approach is much more fine grained than just disable everything approach >> that we have for q35. For i440fx we can do better than that. >> >> >>> > > >> >>> -- >>> MST
Re: [PATCH for-5.1 4/7] target/mips: Add Loongson-3 CPU definition
Hi, Philippe,
The major differences of R3 and R4 are:
1, R4 has complete MIPS VZ ASE (while R3 is incomplete), so very
usable for KVM host;
2, R4 has MSA ASE while R3 hasn't;
3, R4 has cpucfg, rdcsr and wrcsr instructions (similar to cpuid,
rdmsr and wrmsr in X86).
On Wed, Apr 29, 2020 at 4:09 PM Philippe Mathieu-Daudé wrote:
>
> On 4/29/20 5:51 AM, Huacai Chen wrote:
> > Hi, Aleksandar,
> >
> > I've tried translate.google.com, and documents are available here:
> > Loongson-3A R1 (Loongson-3A1000)
> > User Manual Part 1:
> > http://ftp.godson.ac.cn/lemote/3A1000_p1.pdf
> > http://ftp.godson.ac.cn/lemote/Loongson3A1000_processor_user_manual_P1.pdf
> > (Chinese Version)
> > User Manual Part 2:
> > http://ftp.godson.ac.cn/lemote/3A1000_p2.pdf
> > http://ftp.godson.ac.cn/lemote/Loongson3A1000_processor_user_manual_P2.pdf
> > (Chinese Version)
> >
> > Loongson-3A R2 (Loongson-3A2000)
> > User Manual Part 1:
> > http://ftp.godson.ac.cn/lemote/3A2000_p1.pdf
> > http://ftp.godson.ac.cn/lemote/Loongson3A2000_user1.pdf (Chinese Version)
> > User Manual Part 2:
> > http://ftp.godson.ac.cn/lemote/3A2000_p2.pdf
> > http://ftp.godson.ac.cn/lemote/Loongson3A2000_user2.pdf (Chinese Version)
> >
> > Loongson-3A R3 (Loongson-3A3000)
> > User Manual Part 1:
> > http://ftp.godson.ac.cn/lemote/3A3000_p1.pdf
> > http://ftp.godson.ac.cn/lemote/Loongson3A3000_3B3000usermanual1.pdf
> > (Chinese Version)
> > User Manual Part 2:
> > http://ftp.godson.ac.cn/lemote/3A3000_p2.pdf
> > http://ftp.godson.ac.cn/lemote/Loongson3A3000_3B3000usermanual2.pdf
> > (Chinese Version)
> >
> > Loongson-3A R4 (Loongson-3A4000)
> > User Manual Part 1:
> > http://ftp.godson.ac.cn/lemote/3A4000_p1.pdf
> > http://ftp.godson.ac.cn/lemote/3A4000user.pdf (Chinese Version)
> > User Manual Part 2:
> > I'm sorry that it is unavailable now.
>
> Thanks for the translations!
>
> Since we can only review Loongson-3A R3, are there specific features
> from R4 you need that are not available in R3?
>
> >
> > On Wed, Apr 29, 2020 at 2:37 AM Aleksandar Markovic
> > wrote:
> >>
> >> Huacai,
> >>
> >> Can you please do machine translation of the document?
> >>
> >> It can be done via translate.google.com (it accepts pdf files, but
> >> does not have download feature, and workaround is to "print to pdf"...
> >>
> >> Thanks in advance!
> >> Aleksandar
> >>
> >> уто, 28. апр 2020. у 10:26 chen huacai је
> >> написао/ла:
> >>>
> >>> Hi, Philippe,
> >>>
> >>> On Tue, Apr 28, 2020 at 2:34 PM Philippe Mathieu-Daudé
> >>> wrote:
>
> Hi Huacai,
>
> On 4/27/20 11:33 AM, Huacai Chen wrote:
> > Loongson-3 CPU family include Loongson-3A R1/R2/R3/R4 and Loongson-3B
> > R1/R2. Loongson-3A R4 is the newest and its ISA is almost the superset
> > of all others. To reduce complexity, we just define a "Loongson-3A" CPU
> > which is corresponding to Loongson-3A R4. Loongson-3A has CONFIG6 and
> > CONFIG7, so add their bit-fields as well.
>
> Is there a public datasheet for R4? (If possible in English).
> >>> I'm sorry that we only have Chinese datasheet in www.loongson.cn.
> >>>
>
> >
> > Signed-off-by: Huacai Chen
> > Co-developed-by: Jiaxun Yang
> > ---
> > target/mips/cpu.h| 28 ++
> > target/mips/internal.h | 2 ++
> > target/mips/mips-defs.h | 7 --
> > target/mips/translate.c | 2 ++
> > target/mips/translate_init.inc.c | 51
> >
> > 5 files changed, 88 insertions(+), 2 deletions(-)
> >
> > diff --git a/target/mips/cpu.h b/target/mips/cpu.h
> > index 94d01ea..0b3c987 100644
> > --- a/target/mips/cpu.h
> > +++ b/target/mips/cpu.h
> > @@ -940,7 +940,35 @@ struct CPUMIPSState {
> > #define CP0C5_UFR 2
> > #define CP0C5_NFExists 0
> > int32_t CP0_Config6;
> > +int32_t CP0_Config6_rw_bitmask;
> > +#define CP0C6_BPPASS 31
> > +#define CP0C6_KPOS24
> > +#define CP0C6_KE 23
> > +#define CP0C6_VTLBONLY22
> > +#define CP0C6_LASX21
> > +#define CP0C6_SSEN20
> > +#define CP0C6_DISDRTIME 19
> > +#define CP0C6_PIXNUEN 18
> > +#define CP0C6_SCRAND 17
> > +#define CP0C6_LLEXCEN 16
> > +#define CP0C6_DISVC 15
> > +#define CP0C6_VCLRU 14
> > +#define CP0C6_DCLRU 13
> > +#define CP0C6_PIXUEN 12
> > +#define CP0C6_DISBLKLYEN 11
> > +#define CP0C6_UMEMUALEN 10
> > +#define CP0C6_SFBEN 8
> > +#define CP0C6_FLTINT 7
> > +#define CP0C6_VLTINT 6
> > +#define CP0C6_DISBTB 5
> > +#define CP0C6_STPREFCTL 2
> > +#define CP0C6_INSTPREF1
> > +#define CP0C6_DATAPREF0
> > int32_t CP0_Config7;
> > +int64_t CP0_Config7_
Re: [PATCH v5 0/4] introduction of migration_version attribute for VFIO live migration
* Yan Zhao (yan.y.z...@intel.com) wrote: > On Tue, Apr 28, 2020 at 10:14:37PM +0800, Dr. David Alan Gilbert wrote: > > * Yan Zhao (yan.y.z...@intel.com) wrote: > > > On Mon, Apr 27, 2020 at 11:37:43PM +0800, Dr. David Alan Gilbert wrote: > > > > * Yan Zhao (yan.y.z...@intel.com) wrote: > > > > > On Sat, Apr 25, 2020 at 03:10:49AM +0800, Dr. David Alan Gilbert > > > > > wrote: > > > > > > * Yan Zhao (yan.y.z...@intel.com) wrote: > > > > > > > On Tue, Apr 21, 2020 at 08:08:49PM +0800, Tian, Kevin wrote: > > > > > > > > > From: Yan Zhao > > > > > > > > > Sent: Tuesday, April 21, 2020 10:37 AM > > > > > > > > > > > > > > > > > > On Tue, Apr 21, 2020 at 06:56:00AM +0800, Alex Williamson > > > > > > > > > wrote: > > > > > > > > > > On Sun, 19 Apr 2020 21:24:57 -0400 > > > > > > > > > > Yan Zhao wrote: > > > > > > > > > > > > > > > > > > > > > On Fri, Apr 17, 2020 at 07:24:57PM +0800, Cornelia Huck > > > > > > > > > > > wrote: > > > > > > > > > > > > On Fri, 17 Apr 2020 05:52:02 -0400 > > > > > > > > > > > > Yan Zhao wrote: > > > > > > > > > > > > > > > > > > > > > > > > > On Fri, Apr 17, 2020 at 04:44:50PM +0800, Cornelia > > > > > > > > > > > > > Huck wrote: > > > > > > > > > > > > > > On Mon, 13 Apr 2020 01:52:01 -0400 > > > > > > > > > > > > > > Yan Zhao wrote: > > > > > > > > > > > > > > > > > > > > > > > > > > > > > This patchset introduces a migration_version > > > > > > > > > > > > > > > attribute under sysfs > > > > > > > > > of VFIO > > > > > > > > > > > > > > > Mediated devices. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > This migration_version attribute is used to check > > > > > > > > > > > > > > > migration > > > > > > > > > compatibility > > > > > > > > > > > > > > > between two mdev devices. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > Currently, it has two locations: > > > > > > > > > > > > > > > (1) under mdev_type node, > > > > > > > > > > > > > > > which can be used even before device > > > > > > > > > > > > > > > creation, but only for > > > > > > > > > mdev > > > > > > > > > > > > > > > devices of the same mdev type. > > > > > > > > > > > > > > > (2) under mdev device node, > > > > > > > > > > > > > > > which can only be used after the mdev devices > > > > > > > > > > > > > > > are created, but > > > > > > > > > the src > > > > > > > > > > > > > > > and target mdev devices are not necessarily > > > > > > > > > > > > > > > be of the same > > > > > > > > > mdev type > > > > > > > > > > > > > > > (The second location is newly added in v5, in > > > > > > > > > > > > > > > order to keep > > > > > > > > > consistent > > > > > > > > > > > > > > > with the migration_version node for migratable > > > > > > > > > > > > > > > pass-though > > > > > > > > > devices) > > > > > > > > > > > > > > > > > > > > > > > > > > > > What is the relationship between those two > > > > > > > > > > > > > > attributes? > > > > > > > > > > > > > > > > > > > > > > > > > > > (1) is for mdev devices specifically, and (2) is > > > > > > > > > > > > > provided to keep the > > > > > > > > > same > > > > > > > > > > > > > sysfs interface as with non-mdev cases. so (2) is for > > > > > > > > > > > > > both mdev > > > > > > > > > devices and > > > > > > > > > > > > > non-mdev devices. > > > > > > > > > > > > > > > > > > > > > > > > > > in future, if we enable vfio-pci vendor ops, (i.e. a > > > > > > > > > > > > > non-mdev device > > > > > > > > > > > > > is binding to vfio-pci, but is able to register > > > > > > > > > > > > > migration region and do > > > > > > > > > > > > > migration transactions from a vendor provided > > > > > > > > > > > > > affiliate driver), > > > > > > > > > > > > > the vendor driver would export (2) directly, under > > > > > > > > > > > > > device node. > > > > > > > > > > > > > It is not able to provide (1) as there're no mdev > > > > > > > > > > > > > devices involved. > > > > > > > > > > > > > > > > > > > > > > > > Ok, creating an alternate attribute for non-mdev > > > > > > > > > > > > devices makes sense. > > > > > > > > > > > > However, wouldn't that rather be a case (3)? The change > > > > > > > > > > > > here only > > > > > > > > > > > > refers to mdev devices. > > > > > > > > > > > > > > > > > > > > > > > as you pointed below, (3) and (2) serve the same purpose. > > > > > > > > > > > and I think a possible usage is to migrate between a > > > > > > > > > > > non-mdev device and > > > > > > > > > > > an mdev device. so I think it's better for them both to > > > > > > > > > > > use (2) rather > > > > > > > > > > > than creating (3). > > > > > > > > > > > > > > > > > > > > An mdev type is meant to define a software compatible > > > > > > > > > > interface, so in > > > > > > > > > > the case of mdev->mdev migration, doesn't migrating to a > > > > > > > > > > different type > > > > > > > > > > fail the most basic of compatibility tests that we expect > > > > > > > > > > userspace to > > > > > > > > > > perform? IOW, if two md
RE: [PATCH v2 2/2] Improve legacy vbios handling
> -Original Message-
> From: Grzegorz Uriasz
> Sent: 29 April 2020 04:04
> To: qemu-devel@nongnu.org
> Cc: Grzegorz Uriasz ; marma...@invisiblethingslab.com;
> ar...@puzio.waw.pl;
> ja...@bartmin.ski; j.nowa...@student.uw.edu.pl; Stefano Stabellini
> ; Anthony
> Perard ; Paul Durrant ;
> xen-de...@lists.xenproject.org
> Subject: [PATCH v2 2/2] Improve legacy vbios handling
>
> The current method of getting the vbios is broken - it just isn't working on
> any device I've tested -
> the reason
> for this is explained in the previous patch. The vbios is polymorphic and
> getting a proper unmodified
> copy is
> often not possible without reverse engineering the firmware. We don't need an
> unmodified copy for most
> purposes -
> an unmodified copy is only needed for initializing the bios framebuffer and
> providing the bios with a
> corrupted
> copy of the rom won't do any damage as the bios will just ignore the rom.
>
> After the i915 driver takes over the vbios is only needed for reading some
> metadata/configuration
> stuff etc...
> I've tested that not having any kind of vbios in the guest actually works
> fine but on older
> generations of IGD
> there are some slight hiccups. To maximize compatibility the best approach is
> to just copy the results
> of the vbios
> execution directly to the guest. It turns out the vbios is always present on
> an hardcoded memory range
> in a reserved
> memory range from real mode - all we need to do is to memcpy it into the
> guest.
>
> The following patch does 2 things:
> 1) When pci_assign_dev_load_option_rom fails to read the vbios from
> sysfs(this works only when the igd
> is not the
> boot gpu - this is unlikely to happen) it falls back to using /dev/mem to
> copy the vbios directly to
> the guest.
Why bother with sysfs if it is unlikely to work?
> Using /dev/mem should be fine as there is more xen specific pci code which
> also relies on /dev/mem.
> 2) When dealing with IGD in the more generic code we skip the allocation of
> the rom resource - the
> reason for this is to prevent
> a malicious guest from modifying the vbios in the host -> this is needed as
> someone might try to pwn
> the i915 driver in the host by doing so
> (attach igd to guest, guest modifies vbios, the guest is terminated and the
> idg is reattached to the
> host, i915 driver in the host uses data from the modified vbios).
> This is also needed to not overwrite the proper shadow copy made before.
>
> I've tested this patch and it works fine - the guest isn't complaining about
> the missing vbios tables
> and the pci config
> space in the guest looks fine.
>
> Signed-off-by: Grzegorz Uriasz
> ---
> hw/xen/xen_pt.c | 8 +--
> hw/xen/xen_pt_graphics.c | 48 +---
> hw/xen/xen_pt_load_rom.c | 2 +-
> 3 files changed, 52 insertions(+), 6 deletions(-)
>
> diff --git a/hw/xen/xen_pt.c b/hw/xen/xen_pt.c
> index b91082cb8b..ffc3559dd4 100644
> --- a/hw/xen/xen_pt.c
> +++ b/hw/xen/xen_pt.c
> @@ -483,8 +483,12 @@ static int
> xen_pt_register_regions(XenPCIPassthroughState *s, uint16_t *cmd)
> i, r->size, r->base_addr, type);
> }
>
> -/* Register expansion ROM address */
> -if (d->rom.base_addr && d->rom.size) {
> +/*
> + * Register expansion ROM address. If we are dealing with a ROM
> + * shadow copy for legacy vga devices then don't bother to map it
> + * as previous code creates a proper shadow copy
> + */
> +if (d->rom.base_addr && d->rom.size && !(is_igd_vga_passthrough(d))) {
You don't need brackets around the function call.
Paul
> uint32_t bar_data = 0;
>
> /* Re-set BAR reported by OS, otherwise ROM can't be read. */
> diff --git a/hw/xen/xen_pt_graphics.c b/hw/xen/xen_pt_graphics.c
> index a3bc7e3921..fe0ef2685c 100644
> --- a/hw/xen/xen_pt_graphics.c
> +++ b/hw/xen/xen_pt_graphics.c
> @@ -129,7 +129,7 @@ int xen_pt_unregister_vga_regions(XenHostPCIDevice *dev)
> return 0;
> }
>
> -static void *get_vgabios(XenPCIPassthroughState *s, int *size,
> +static void *get_sysfs_vgabios(XenPCIPassthroughState *s, int *size,
> XenHostPCIDevice *dev)
> {
> return pci_assign_dev_load_option_rom(&s->dev, size,
> @@ -137,6 +137,45 @@ static void *get_vgabios(XenPCIPassthroughState *s, int
> *size,
>dev->dev, dev->func);
> }
>
> +static void xen_pt_direct_vbios_copy(XenPCIPassthroughState *s, Error **errp)
> +{
> +int fd = -1;
> +void *guest_bios = NULL;
> +void *host_vbios = NULL;
> +/* This is always 32 pages in the real mode reserved region */
> +int bios_size = 32 << XC_PAGE_SHIFT;
> +int vbios_addr = 0xc;
> +
> +fd = open("/dev/mem", O_RDONLY);
> +if (fd == -1) {
> +error_setg(errp, "Can't open /dev/mem: %s", strerror(errno));
> +return;
> +}
> +host_vbios = mmap(NULL, bios_size,
> +PROT
[PATCH 2/2] target/mips/kvm: Assert unreachable code is not used
This code must not be used outside of KVM. Abort if it is.
Signed-off-by: Philippe Mathieu-Daudé
---
target/mips/kvm.c | 8 ++--
1 file changed, 2 insertions(+), 6 deletions(-)
diff --git a/target/mips/kvm.c b/target/mips/kvm.c
index de3e26ef1f..050bfbd7fa 100644
--- a/target/mips/kvm.c
+++ b/target/mips/kvm.c
@@ -196,9 +196,7 @@ int kvm_mips_set_interrupt(MIPSCPU *cpu, int irq, int level)
CPUState *cs = CPU(cpu);
struct kvm_mips_interrupt intr;
-if (!kvm_enabled()) {
-return 0;
-}
+assert(kvm_enabled());
intr.cpu = -1;
@@ -219,9 +217,7 @@ int kvm_mips_set_ipi_interrupt(MIPSCPU *cpu, int irq, int
level)
CPUState *dest_cs = CPU(cpu);
struct kvm_mips_interrupt intr;
-if (!kvm_enabled()) {
-return 0;
-}
+assert(kvm_enabled());
intr.cpu = dest_cs->cpu_index;
--
2.21.1
[PATCH 1/2] hw/mips/mips_int: De-duplicate KVM interrupt delivery
Refactor duplicated code in a single place.
Signed-off-by: Philippe Mathieu-Daudé
---
hw/mips/mips_int.c | 11 +++
1 file changed, 3 insertions(+), 8 deletions(-)
diff --git a/hw/mips/mips_int.c b/hw/mips/mips_int.c
index 796730b11d..4a1bf846da 100644
--- a/hw/mips/mips_int.c
+++ b/hw/mips/mips_int.c
@@ -47,17 +47,12 @@ static void cpu_mips_irq_request(void *opaque, int irq, int
level)
if (level) {
env->CP0_Cause |= 1 << (irq + CP0Ca_IP);
-
-if (kvm_enabled() && irq == 2) {
-kvm_mips_set_interrupt(cpu, irq, level);
-}
-
} else {
env->CP0_Cause &= ~(1 << (irq + CP0Ca_IP));
+}
-if (kvm_enabled() && irq == 2) {
-kvm_mips_set_interrupt(cpu, irq, level);
-}
+if (kvm_enabled() && irq == 2) {
+kvm_mips_set_interrupt(cpu, irq, level);
}
if (env->CP0_Cause & CP0Ca_IP_mask) {
--
2.21.1
[PATCH 0/2] mips: Minor simplifications for KVM use
A pair of trivial patches while reviewing Huacai's "KVM target support for MIPS64" series. Philippe Mathieu-Daudé (2): hw/mips/mips_int: De-duplicate KVM interrupt delivery target/mips/kvm: Assert unreachable code is not used hw/mips/mips_int.c | 11 +++ target/mips/kvm.c | 8 ++-- 2 files changed, 5 insertions(+), 14 deletions(-) -- 2.21.1
Re: [Virtio-fs] [PATCH] virtiofsd: Show submounts
On 28.04.20 21:15, Dr. David Alan Gilbert wrote:
> * Miklos Szeredi (mszer...@redhat.com) wrote:
>> On Tue, Apr 28, 2020 at 4:52 PM Stefan Hajnoczi wrote:
>>>
>>> On Mon, Apr 27, 2020 at 06:59:02PM +0100, Dr. David Alan Gilbert wrote:
* Max Reitz (mre...@redhat.com) wrote:
> Currently, setup_mounts() bind-mounts the shared directory without
> MS_REC. This makes all submounts disappear.
>
> Pass MS_REC so that the guest can see submounts again.
Thanks!
> Fixes: 3ca8a2b1c83eb185c232a4e87abbb65495263756
Should this actually be 5baa3b8e95064c2434bd9e2f312edd5e9ae275dc ?
> Signed-off-by: Max Reitz
> ---
> tools/virtiofsd/passthrough_ll.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/tools/virtiofsd/passthrough_ll.c
> b/tools/virtiofsd/passthrough_ll.c
> index 4c35c95b25..9d7f863e66 100644
> --- a/tools/virtiofsd/passthrough_ll.c
> +++ b/tools/virtiofsd/passthrough_ll.c
> @@ -2643,7 +2643,7 @@ static void setup_mounts(const char *source)
> int oldroot;
> int newroot;
>
> -if (mount(source, source, NULL, MS_BIND, NULL) < 0) {
> +if (mount(source, source, NULL, MS_BIND | MS_REC, NULL) < 0) {
> fuse_log(FUSE_LOG_ERR, "mount(%s, %s, MS_BIND): %m\n", source,
> source);
> exit(1);
> }
Do we want MS_SLAVE to pick up future mounts that might happenf rom the
host?
>>>
>>> There are two separate concepts:
>>>
>>> 1. Mount namespaces. The virtiofsd process is sandboxed and lives in
>>>its own mount namespace. Therefore it does not share the mounts that
>>>the rest of the host system sees.
>>>
>>> 2. Propagation type. This is related to bind mounts so that mount
>>>operations that happen in one bind-mounted location can also appear
>>>in other bind-mounted locations.
>>>
>>> Since virtiofsd is in a separate mount namespace, does the propagation
>>> type even have any effect?
>>
>> It's a complicated thing. Current setup results in propagation
>> happening to the cloned namespace, but not to the bind mounted root.
>>
>> Why? Because setting mounts "slave" after unshare, results in the
>> propagation being stopped at that point. To make it propagate
>> further, change it back to "shared". Note: the result changing to
>> "slave" and then to "shared" results in breaking the backward
>> propagation to the original namespace, but allowing propagation
>> further down the chain.
>
> Do you mean on the "/" ?
>
> So our current sequence is:
>
>(new namespace)
> 1)if (mount(NULL, "/", NULL, MS_REC | MS_SLAVE, NULL) < 0) {
> 2) if (mount("proc", "/proc", "proc",
>
> 3) if (mount(source, source, NULL, MS_BIND | MS_REC, NULL) < 0) {
> 4) (chdir newroot, pivot, chdir oldroot)
> 5) if (mount("", ".", "", MS_SLAVE | MS_REC, NULL) < 0) {
> 6) if (umount2(".", MNT_DETACH) < 0) {
>
> So are you saying we need a:
>if (mount(NULL, "/", NULL, MS_REC | MS_SHARED, NULL) < 0) {
>
> and can this go straight after (1) ?
Isn’t MS_SHARED and MS_SLAVE mutually exclusive, that is, both are just
different propagation types? So shouldn’t putting this after (1) be
effectively the same as replacing (1)?
Max
signature.asc
Description: OpenPGP digital signature
Re: [PATCH 1/2] hw/mips/mips_int: De-duplicate KVM interrupt delivery
Hi, Philippe,
On Wed, Apr 29, 2020 at 4:30 PM Philippe Mathieu-Daudé wrote:
>
> Refactor duplicated code in a single place.
>
> Signed-off-by: Philippe Mathieu-Daudé
> ---
> hw/mips/mips_int.c | 11 +++
> 1 file changed, 3 insertions(+), 8 deletions(-)
>
> diff --git a/hw/mips/mips_int.c b/hw/mips/mips_int.c
> index 796730b11d..4a1bf846da 100644
> --- a/hw/mips/mips_int.c
> +++ b/hw/mips/mips_int.c
> @@ -47,17 +47,12 @@ static void cpu_mips_irq_request(void *opaque, int irq,
> int level)
>
> if (level) {
> env->CP0_Cause |= 1 << (irq + CP0Ca_IP);
> -
> -if (kvm_enabled() && irq == 2) {
> -kvm_mips_set_interrupt(cpu, irq, level);
> -}
> -
> } else {
> env->CP0_Cause &= ~(1 << (irq + CP0Ca_IP));
> +}
Since the if-else has become one line, so can we remove { and } here?
>
> -if (kvm_enabled() && irq == 2) {
> -kvm_mips_set_interrupt(cpu, irq, level);
> -}
> +if (kvm_enabled() && irq == 2) {
> +kvm_mips_set_interrupt(cpu, irq, level);
> }
>
> if (env->CP0_Cause & CP0Ca_IP_mask) {
> --
> 2.21.1
>
>
--
Huacai Chen
Re: [PATCH 1/2] softfloat: m68k: infinity is a valid encoding
Le 28/04/2020 à 19:17, KONRAD Frederic a écrit :
> The MC68881 say about infinities (3.2.4):
>
> "*For the extended precision format, the most significant bit of the
> mantissa (the integer bit) is a don't care."
>
> https://www.nxp.com/docs/en/reference-manual/MC68881UM.pdf
As we use 68040 I refer to:
https://www.nxp.com/files-static/archives/doc/ref_manual/M68000PRM.pdf
>
> The m68k extended format is implemented with the floatx80 and
> floatx80_invalid_encoding currently treats 0x7fff as
> an invalid encoding. This patch fixes floatx80_invalid_encoding so it
> accepts that the most significant bit of the mantissa can be 0.
>
> This bug can be revealed with the following code which pushes extended
> infinity on the stack as a double and then reloads it as a double. It
> should normally be converted and read back as infinity and is currently
> read back as nan:
>
> .global _start
> .text
> _start:
> lea val, %a0
> lea fp, %fp
> fmovex (%a0), %fp0
> fmoved %fp0, %fp@(-8)
> fmoved %fp@(-8), %fp0
> end:
> bra end
>
> .align 0x4
> val:
> .fill 1, 4, 0x7fff
> .fill 1, 4, 0x
> .fill 1, 4, 0x
> .align 0x4
> .fill 0x100, 1, 0
> fp:
>
> -
>
> (gdb) tar rem :1234
> Remote debugging using :1234
> _start () at main.S:5
> 5 lea val, %a0
> (gdb) display $fp0
> 1: $fp0 = nan(0x)
> (gdb) si
> 6 lea fp, %fp
> 1: $fp0 = nan(0x)
> (gdb) si
> _start () at main.S:7
> 7 fmovex (%a0), %fp0
> 1: $fp0 = nan(0x)
> (gdb) si
> 8 fmoved %fp0, %fp@(-8)
> 1: $fp0 = inf
> (gdb) si
> 9 fmoved %fp@(-8), %fp0
> 1: $fp0 = inf
> (gdb) si
> end () at main.S:12
> 12 bra end
> 1: $fp0 = nan(0xf800)
> (gdb) x/1xg $fp-8
> 0x4120 : 0x7fff
>
> Signed-off-by: KONRAD Frederic
> ---
> include/fpu/softfloat.h | 5 +
> 1 file changed, 5 insertions(+)
>
> diff --git a/include/fpu/softfloat.h b/include/fpu/softfloat.h
> index ecb8ba0..dc80298 100644
> --- a/include/fpu/softfloat.h
> +++ b/include/fpu/softfloat.h
> @@ -688,7 +688,12 @@ static inline int floatx80_is_any_nan(floatx80 a)
>
> **/
> static inline bool floatx80_invalid_encoding(floatx80 a)
> {
> +#if defined(TARGET_M68K)
> +return (a.low & (1ULL << 63)) == 0 && (((a.high & 0x7FFF) != 0)
> + && (a.high != 0x7FFF));
> +#else
> return (a.low & (1ULL << 63)) == 0 && (a.high & 0x7FFF) != 0;
> +#endif
> }
>
> #define floatx80_zero make_floatx80(0x, 0xLL)
>
This is denormalized numbers and should generate an exception.
I tried something like that in the past:
https://patchew.org/QEMU/20170207005930.28327-1-laur...@vivier.eu/20170207005930.28327-3-laur...@vivier.eu/
Pierre tried recently:
https://patchew.org/QEMU/1615bbe5-3033-3b76-5cfb-52e343dc4...@freepascal.org/
See "1.6.2 Denormalized Numbers" in M68000 FAMILY PROGRAMMER’S REFERENCE
MANUAL.
"Since the extended-precision data format has an explicit integer bit, a
number can be formatted with a nonzero exponent, less than the maximum
value, and a zero integer bit. The IEEE 754 standard does not define a
zero integer bit. Such a number is an unnormalized number. Hardware does
not directly support denormalized and unnormalized numbers, but
implicitly supports them by trapping them as unimplemented data types,
allowing efficient conversion in software."
But m68k FPU exceptions are not currently implemented in QEMU.
Thanks,
Laurent
Re: [PATCH 1/2] softfloat: m68k: infinity is a valid encoding
Le 28/04/2020 à 20:43, Alex Bennée a écrit : > > KONRAD Frederic writes: > >> The MC68881 say about infinities (3.2.4): >> >> "*For the extended precision format, the most significant bit of the >> mantissa (the integer bit) is a don't care." >> >> https://www.nxp.com/docs/en/reference-manual/MC68881UM.pdf >> >> The m68k extended format is implemented with the floatx80 and >> floatx80_invalid_encoding currently treats 0x7fff as >> an invalid encoding. This patch fixes floatx80_invalid_encoding so it >> accepts that the most significant bit of the mantissa can be 0. >> >> This bug can be revealed with the following code which pushes extended >> infinity on the stack as a double and then reloads it as a double. It >> should normally be converted and read back as infinity and is currently >> read back as nan: > > Do you have any real HW on which you could record some .ref files for > the various multiarch float tests we have (float_convs/float_madds)? > Does this different of invalid encoding show up when you add them? On my side, in the past when I started to implement m68k FPU, I used TestFloat and SoftFloat I have ported to m68k and I compare the result in QEMU and in a Quadra 800. https://github.com/vivier/m68k-testfloat https://github.com/vivier/m68k-softfloat I also used the gcc and libc testsuite to detect problems but this was a very slow process... I have also ported RISU to m68k, but I didn't add FPU test in it (does it support FPU test?). Thanks, Laurent
Re: [PATCH 4/5] ramfb: add sanity checks to ramfb_create_display_surface
Hi, > > Not fully sure we can do that without breaking something, might be a > > negative stride is used for upside down images (last scanline comes > > first in memory). > > Ugh... Upside down images???... Well, OK, I guess. :) Well, in the unix world (x11, wayland) x=0,y=0 is the upper left corner. In the windows world x=0,y=0 is the lower left corner, in opengl too. If you don't handle this correctly your guest display might show up upside down ;) qxl uses negative strides to signal that. Looking at the code I see qxl handles this locally (grep for qxl_stride in qxl-render.c), it doesn't propagate into ui/console.c, so it should be safe to change the qemu_create_displaysurface_from() arguments to unsigned from qxl point of view. take care, Gerd
Re: [Virtio-fs] [PATCH] virtiofsd: Show submounts
On Wed, Apr 29, 2020 at 10:31 AM Max Reitz wrote:
>
> On 28.04.20 21:15, Dr. David Alan Gilbert wrote:
> > So are you saying we need a:
> >if (mount(NULL, "/", NULL, MS_REC | MS_SHARED, NULL) < 0) {
> >
> > and can this go straight after (1) ?
>
> Isn’t MS_SHARED and MS_SLAVE mutually exclusive, that is, both are just
> different propagation types? So shouldn’t putting this after (1) be
> effectively the same as replacing (1)?
No, think of it more like a set of groups (mounts in a group have the
same "shared:$ID" tag in /proc/self/mountinfo) and these groups being
arranged into a tree, where child groups get propagation from the
parent group (mounts have a "master:$PARENT_ID" tag), but not the
other way round.
So if a mount is part of a shared group and this group is also the
child of another shared group, then it will have both a "shared:$ID"
and a "master:$PARENT_ID" tag in /proc/self/mountinfo.
For the gory details see Documentation/filesystems/sharedsubtree.txt
Thanks,
Miklos
[PATCH v3 0/3] fetch the alignment of device dax
This series adds libdaxctl support and fetchs the alignment of device dax through libdaxctl [1] APIs. QEMU uses mmap(2) to maps vNVDIMM backends and aligns the mapping address to the page size (getpagesize(2)) by default. However, some types of backends may require an alignment different than the page size. The 'align' option is provided to memory-backend-file to allow users to specify the proper alignment. For device dax (e.g., /dev/dax0.0), the 'align' option needs to match the alignment requirement of the device dax, which can be fetched through the APIs of libdaxctl version 57 or up. [1] Libdaxctl is a part of ndctl project. The project's repository is: https://github.com/pmem/ndctl Changelog: v3: - Per Joao's suggestion, require libdaxctl version 57 or up. - Per Joao's suggestion, suggest to query the @align with daxctl tool in docs/nvdimm.txt. v2: - Per Paolo and Dan's suggestions, fetch the alignment of device dax through libdaxctl APIs. v1: - The initial version. Fetch the alignment through "/sys/dev/char/%d:%d/device/align". Jingqi Liu (3): exec: fetch the alignment of Linux devdax pmem character device nodes docs/nvdimm: add description of alignment requirement of device dax configure: add libdaxctl support configure | 29 ++ docs/nvdimm.txt | 10 + exec.c | 54 - 3 files changed, 92 insertions(+), 1 deletion(-) -- 2.17.1
Re: [PATCH 08/17] s390x/cpumodel: Fix UI to CPU features pcc-cmac-{aes,eaes}-256
On 28.04.20 19:13, David Hildenbrand wrote: > On 28.04.20 18:34, Markus Armbruster wrote: >> Both s390_features[S390_FEAT_PCC_CMAC_AES_256].name and >> s390_features[S390_FEAT_PCC_CMAC_EAES_256].name is >> "pcc-cmac-eaes-256". The former is obviously a pasto. >> >> Impact: >> >> * s390_feat_bitmap_to_ascii() misidentifies S390_FEAT_PCC_CMAC_AES_256 >> as "pcc-cmac-eaes-256". Affects QMP commands query-cpu-definitions, >> query-cpu-model-expansion, query-cpu-model-baseline, >> query-cpu-model-comparison, and the error message when >> s390_realize_cpu_model() fails in check_compatibility(). >> >> * s390_realize_cpu_model() misidentifies it in check_consistency() >> warnings. >> >> * s390_cpu_list() likewise. Affects -cpu help. >> >> * s390_cpu_model_register_props() creates CPU property >> "pcc-cmac-eaes-256" twice. The second one fails, but the error is >> ignored (a later commit will change that). Results in a single >> property "pcc-cmac-eaes-256" with the description for >> S390_FEAT_PCC_CMAC_AES_256, and no property for >> S390_FEAT_PCC_CMAC_EAES_256. CPU properties are visible in CLI -cpu >> and -device, QMP & HMP device_add, QMP device-list-properties, and >> QOM introspection. >> >> Fix by deleting the wayward 'e'. > > Very nice catch - thanks! > > While this sounds very bad, it's luckily not that bad in practice > (currently). > > The feature (or rather, both features) is part of the feature group > "msa4". As long as we have all sub-features part of that group (which is > usually the case), we will always indicate "msa4" to the user, instead > of all the separate sub-features. So, expansion, baseline, comparison > will usually only work with "msa4". > > (in addition, current KVM is not capable of actually masking off these > sub-features, so it will still, always see the feature, even if not > explicitly specified via "-cpu X,pcc-cmac-aes-256=on) > > I think we should do stable backports. makes sense, but I would like to do some testing upfront (old QEMU <-> new QEMU
Re: [PATCH 10/17] e1000: Don't run e1000_instance_init() twice
On 2020/4/29 上午12:34, Markus Armbruster wrote: QOM object initialization runs .instance_init() for the type and all its supertypes; see object_init_with_type(). Both TYPE_E1000_BASE and its concrete subtypes set .instance_init() to e1000_instance_init(). For the concrete subtypes, it duly gets run twice. The second run fails, but the error gets ignored (a later commit will change that). Remove it from the subtypes. Cc: Jason Wang Signed-off-by: Markus Armbruster Acked-by: Jason Wang --- hw/net/e1000.c | 1 - 1 file changed, 1 deletion(-) diff --git a/hw/net/e1000.c b/hw/net/e1000.c index 2a69eee63f..0d2c2759e3 100644 --- a/hw/net/e1000.c +++ b/hw/net/e1000.c @@ -1824,7 +1824,6 @@ static void e1000_register_types(void) type_info.parent = TYPE_E1000_BASE; type_info.class_data = (void *)info; type_info.class_init = e1000_class_init; -type_info.instance_init = e1000_instance_init; type_register(&type_info); }
[PATCH v3 1/3] exec: fetch the alignment of Linux devdax pmem character device nodes
If the backend file is devdax pmem character device, the alignment
specified by the option 'align=NUM' in the '-object memory-backend-file'
needs to match the alignment requirement of the devdax pmem character device.
This patch uses the interfaces of libdaxctl to fetch the devdax pmem file
'align', so that we can compare it with the NUM of 'align=NUM'.
The NUM needs to be larger than or equal to the devdax pmem file 'align'.
It also fixes the problem that mmap() returns failure in qemu_ram_mmap()
when the NUM of 'align=NUM' is less than the devdax pmem file 'align'.
Suggested-by: Dan Williams
Reviewed-by: Joao Martins
Signed-off-by: Jingqi Liu
---
exec.c | 54 +-
1 file changed, 53 insertions(+), 1 deletion(-)
diff --git a/exec.c b/exec.c
index de9d949902..2c3444e47e 100644
--- a/exec.c
+++ b/exec.c
@@ -77,6 +77,10 @@
#include "monitor/monitor.h"
+#ifdef CONFIG_LIBDAXCTL
+#include
+#endif
+
//#define DEBUG_SUBPAGE
#if !defined(CONFIG_USER_ONLY)
@@ -1736,6 +1740,46 @@ static int64_t get_file_size(int fd)
return size;
}
+static int64_t get_file_align(int fd)
+{
+int64_t align = -1;
+#if defined(__linux__) && defined(CONFIG_LIBDAXCTL)
+struct stat st;
+
+if (fstat(fd, &st) < 0) {
+return -errno;
+}
+
+/* Special handling for devdax character devices */
+if (S_ISCHR(st.st_mode)) {
+g_autofree char *path = NULL;
+g_autofree char *rpath = NULL;
+struct daxctl_ctx *ctx;
+struct daxctl_region *region;
+int rc = 0;
+
+path = g_strdup_printf("/sys/dev/char/%d:%d",
+major(st.st_rdev), minor(st.st_rdev));
+rpath = realpath(path, NULL);
+
+rc = daxctl_new(&ctx);
+if (rc) {
+return -1;
+}
+
+daxctl_region_foreach(ctx, region) {
+if (strstr(rpath, daxctl_region_get_path(region))) {
+align = daxctl_region_get_align(region);
+break;
+}
+}
+daxctl_unref(ctx);
+}
+#endif /* defined(__linux__) && defined(CONFIG_LIBDAXCTL) */
+
+return align;
+}
+
static int file_ram_open(const char *path,
const char *region_name,
bool *created,
@@ -2275,7 +2319,7 @@ RAMBlock *qemu_ram_alloc_from_fd(ram_addr_t size,
MemoryRegion *mr,
{
RAMBlock *new_block;
Error *local_err = NULL;
-int64_t file_size;
+int64_t file_size, file_align;
/* Just support these ram flags by now. */
assert((ram_flags & ~(RAM_SHARED | RAM_PMEM)) == 0);
@@ -2311,6 +2355,14 @@ RAMBlock *qemu_ram_alloc_from_fd(ram_addr_t size,
MemoryRegion *mr,
return NULL;
}
+file_align = get_file_align(fd);
+if (file_align > 0 && mr && file_align > mr->align) {
+error_setg(errp, "backing store align 0x%" PRIx64
+ " is larger than 'align' option 0x" RAM_ADDR_FMT,
+ file_align, mr->align);
+return NULL;
+}
+
new_block = g_malloc0(sizeof(*new_block));
new_block->mr = mr;
new_block->used_length = size;
--
2.17.1
Re: [PULL 00/32] Miscellaneous patches for 2020-04-29
Patchew URL: https://patchew.org/QEMU/20200429072048.29963-1-arm...@redhat.com/
Hi,
This series seems to have some coding style problems. See output below for
more information:
Message-id: 20200429072048.29963-1-arm...@redhat.com
Subject: [PULL 00/32] Miscellaneous patches for 2020-04-29
Type: series
=== TEST SCRIPT BEGIN ===
#!/bin/bash
git rev-parse base > /dev/null || exit 0
git config --local diff.renamelimit 0
git config --local diff.renames True
git config --local diff.algorithm histogram
./scripts/checkpatch.pl --mailback base..
=== TEST SCRIPT END ===
Switched to a new branch 'test'
bbf2736 qemu-option: pass NULL rather than 0 to the id of qemu_opts_set()
3736e38 libqos: Give get_machine_allocator() internal linkage
40a5fe7 fuzz: Simplify how we compute available machines and types
08bd8b8 Makefile: Drop unused, broken target recurse-fuzz
96b5543 smbus: Fix spd_data_generate() for number of banks > 2
a2b3bfe bamboo, sam460ex: Tidy up error message for unsupported RAM size
8cc9661 smbus: Fix spd_data_generate() error API violation
c6b2e22 sam460ex: Suppress useless warning on -m 32 and -m 64
8676301 qga: Fix qmp_guest_suspend_{disk, ram}() error handling
f2ba556 qga: Fix qmp_guest_get_memory_blocks() error handling
1e52928 tests/test-logging: Fix test for -dfilter 0..0x
42be747 migration/colo: Fix qmp_xen_colo_do_checkpoint() error handling
69a4ebd io: Fix qio_channel_socket_close() error handling
21ca33c xen/pt: Fix flawed conversion to realize()
0397c8f virtio-net: Fix duplex=... and speed=... error handling
c849702 bochs-display: Fix vgamem=SIZE error handling
4734313 fdc: Fix fallback=auto error handling
d40f549 arm/virt: Fix virt_machine_device_plug_cb() error API violation
669233d cpus: Proper range-checking for -icount shift=N
33e02df cpus: Fix configure_icount() error API violation
5e67774 block/file-posix: Fix check_cache_dropped() error handling
670e190 cryptodev: Fix cryptodev_builtin_cleanup() error API violation
3e5b9b0 qemu-img: Reject broken -o ""
f08bae7 qemu-img: Move is_valid_option_list() to qemu-img.c and rewrite
536796f qemu-img: Factor out accumulate_options() helper
eb08b54 qemu-option: Avoid has_help_option() in qemu_opts_parse_noisily()
f866f2c test-qemu-opts: Simplify test_has_help_option() after bug fix
42681dc qemu-option: Fix has_help_option()'s sloppy parsing
4cf9436 qemu-option: Fix sloppy recognition of "id=..." after ", , "
9b629d5 qemu-options: Factor out get_opt_name_value() helper
a85727e tests-qemu-opts: Cover has_help_option(), qemu_opt_has_help_opt()
fc0770d various: Remove suspicious '\' character outside of #define in C code
=== OUTPUT BEGIN ===
1/32 Checking commit fc0770db08c1 (various: Remove suspicious '\' character
outside of #define in C code)
2/32 Checking commit a85727e34525 (tests-qemu-opts: Cover has_help_option(),
qemu_opt_has_help_opt())
WARNING: Block comments use a leading /* on a separate line
#44: FILE: tests/test-qemu-opts.c:752:
+{ "a,b,,help", false /* BUG */, true, true },
WARNING: Block comments use a leading /* on a separate line
#45: FILE: tests/test-qemu-opts.c:753:
+{ "a,b,,?", false /* BUG */, true, true },
total: 0 errors, 2 warnings, 56 lines checked
Patch 2/32 has style problems, please review. If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.
3/32 Checking commit 9b629d54ce8a (qemu-options: Factor out
get_opt_name_value() helper)
4/32 Checking commit 4cf9436358f5 (qemu-option: Fix sloppy recognition of
"id=..." after ", , ")
5/32 Checking commit 42681dc0fb50 (qemu-option: Fix has_help_option()'s sloppy
parsing)
6/32 Checking commit f866f2c224ff (test-qemu-opts: Simplify
test_has_help_option() after bug fix)
7/32 Checking commit eb08b546645f (qemu-option: Avoid has_help_option() in
qemu_opts_parse_noisily())
8/32 Checking commit 536796f458ef (qemu-img: Factor out accumulate_options()
helper)
9/32 Checking commit f08bae71a126 (qemu-img: Move is_valid_option_list() to
qemu-img.c and rewrite)
ERROR: suspect code indent for conditional statements (4, 4)
#63: FILE: qemu-img.c:243:
+for (i = len; i > 0 && optarg[i - 1] == ','; i--) {
+}
total: 1 errors, 0 warnings, 67 lines checked
Patch 9/32 has style problems, please review. If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.
10/32 Checking commit 3e5b9b0b2c62 (qemu-img: Reject broken -o "")
ERROR: trailing whitespace
#51: FILE: qemu-img.c:234:
+ * $
total: 1 errors, 0 warnings, 18 lines checked
Patch 10/32 has style problems, please review. If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.
11/32 Checking commit 670e190178ba (cryptodev: Fix cryptodev_builtin_cleanup()
error API violation)
12/32 Checking commit 5e677748f96e (block/file-posix: Fix check_cache_dropped()
error handling)
13/32 Checking commit 33e02df34d16 (cpus: Fix configure_icount() error AP
[PATCH v3 2/3] docs/nvdimm: add description of alignment requirement of device dax
For device dax (e.g., /dev/dax0.0), the NUM of 'align=NUM' option needs to match the alignment requirement of the device dax. It must be larger than or equal to the 'align' of device dax. Reviewed-by: Joao Martins Signed-off-by: Jingqi Liu --- docs/nvdimm.txt | 10 ++ 1 file changed, 10 insertions(+) diff --git a/docs/nvdimm.txt b/docs/nvdimm.txt index 362e99109e..c2c6e441b3 100644 --- a/docs/nvdimm.txt +++ b/docs/nvdimm.txt @@ -132,6 +132,16 @@ address to the page size (getpagesize(2)) by default. However, some types of backends may require an alignment different than the page size. In that case, QEMU v2.12.0 and later provide 'align' option to memory-backend-file to allow users to specify the proper alignment. +For device dax (e.g., /dev/dax0.0), this alignment needs to match the +alignment requirement of the device dax. The NUM of 'align=NUM' option +must be larger than or equal to the 'align' of device dax. +We can use one of the following commands to show the 'align' of device dax. + +ndctl list -X +daxctl list -R + +In order to get the proper 'align' of device dax, you need to install +the library 'libdaxctl'. For example, device dax require the 2 MB alignment, so we can use following QEMU command line options to use it (/dev/dax0.0) as the -- 2.17.1
[PATCH v3 3/3] configure: add libdaxctl support
Add a pair of configure options --{enable,disable}-libdaxctl to control
whether QEMU is compiled with libdaxctl [1]. Libdaxctl is a utility
library for managing the device dax subsystem.
QEMU uses mmap(2) to maps vNVDIMM backends and aligns the mapping
address to the page size (getpagesize(2)) by default. However, some
types of backends may require an alignment different than the page
size. The 'align' option is provided to memory-backend-file to allow
users to specify the proper alignment.
For device dax (e.g., /dev/dax0.0), the 'align' option needs to match
the alignment requirement of the device dax, which can be fetched
through the APIs of libdaxctl version 57 or up.
[1] Libdaxctl is a part of ndctl project.
The project's repository is: https://github.com/pmem/ndctl
For more information about libdaxctl APIs, you can refer to the
comments in source code of: pmem/ndctl/daxctl/lib/libdaxctl.c.
Reviewed-by: Joao Martins
Signed-off-by: Jingqi Liu
---
configure | 29 +
1 file changed, 29 insertions(+)
diff --git a/configure b/configure
index e225a1e3ff..d2418084c1 100755
--- a/configure
+++ b/configure
@@ -509,6 +509,7 @@ libpmem=""
default_devices="yes"
plugins="no"
fuzzing="no"
+libdaxctl=""
supported_cpu="no"
supported_os="no"
@@ -1601,6 +1602,10 @@ for opt do
;;
--gdb=*) gdb_bin="$optarg"
;;
+ --enable-libdaxctl) libdaxctl=yes
+ ;;
+ --disable-libdaxctl) libdaxctl=no
+ ;;
*)
echo "ERROR: unknown option $opt"
echo "Try '$0 --help' for more information"
@@ -1894,6 +1899,7 @@ disabled with --disable-FEATURE, default is enabled if
available:
debug-mutex mutex debugging support
libpmem libpmem support
xkbcommon xkbcommon support
+ libdaxctl libdaxctl support
NOTE: The object files are built at the place where configure is launched
EOF
@@ -6190,6 +6196,24 @@ if test "$libpmem" != "no"; then
fi
fi
+##
+# check for libdaxctl
+
+if test "$libdaxctl" != "no"; then
+ if $pkg_config --atleast-version=57 "libdaxctl"; then
+ libdaxctl="yes"
+ libdaxctl_libs=$($pkg_config --libs libdaxctl)
+ libdaxctl_cflags=$($pkg_config --cflags libdaxctl)
+ libs_softmmu="$libs_softmmu $libdaxctl_libs"
+ QEMU_CFLAGS="$QEMU_CFLAGS $libdaxctl_cflags"
+ else
+ if test "$libdaxctl" = "yes" ; then
+ feature_not_found "libdaxctl" "Install libdaxctl"
+ fi
+ libdaxctl="no"
+ fi
+fi
+
##
# check for slirp
@@ -6767,6 +6791,7 @@ echo "parallels support $parallels"
echo "sheepdog support $sheepdog"
echo "capstone $capstone"
echo "libpmem support $libpmem"
+echo "libdaxctl support $libdaxctl"
echo "libudev $libudev"
echo "default devices $default_devices"
echo "plugin support$plugins"
@@ -7590,6 +7615,10 @@ if test "$libpmem" = "yes" ; then
echo "CONFIG_LIBPMEM=y" >> $config_host_mak
fi
+if test "$libdaxctl" = "yes" ; then
+ echo "CONFIG_LIBDAXCTL=y" >> $config_host_mak
+fi
+
if test "$bochs" = "yes" ; then
echo "CONFIG_BOCHS=y" >> $config_host_mak
fi
--
2.17.1
Re: [PATCH v4 0/3] qcow2: Allow resize of images with internal snapshots
On 28.04.20 21:26, Eric Blake wrote: > Re-posting this to make Max' life easier when rebasing on top of Kevin's work. > > Based-on: <20200424125448.63318-1-kw...@redhat.com> > [PATCH v7 00/10] block: Fix resize (extending) of short overlays > > In v4: > - patch 1: fold in Max's touchups to my v3 > - patch 1: resolve merge conflict on top of Kevin's block-next branch > (truncate signature change) > - patch 2: resolve semantic conflict (truncate signature change) > > 001/3:[0004] [FC] 'block: Add blk_new_with_bs() helper' > 002/3:[0002] [FC] 'qcow2: Allow resize of images with internal snapshots' > 003/3:[] [--] 'qcow2: Tweak comment about bitmaps vs. resize' Thanks, I’ve updated my branch: https://git.xanclic.moe/XanClic/qemu/commits/branch/block-next (A commit with two Message-Id tags is interesting :)) Max signature.asc Description: OpenPGP digital signature
