Re: [Qemu-devel] [PATCH v2] vl.c: print error message if load fw_cfg file failed
Hello Philippe,
Philippe Mathieu-Daudé 于2018年10月9日周二 下午1:52写道：
> Hi Li,
>
> On 09/10/2018 04:39, Li Qiang wrote:
> > It makes sense to print the error message while reading
> > file failed.
>
> OK
>
> >
> > Change since v1:
> > free error
>
> Changes are useful for reviewer, but not in the git history.
> You can have them automatically stripped if you place them below the
> next '---' separator.
>
>
Thanks for your advice.
> Hopefully the maintainer taking this patch can clean this up.
>
> >
> > Signed-off-by: Li Qiang
> > Reviewed-by: Philippe Mathieu-Daudé
> > ---
>
> Here goes comment not worth to stay forever in git history:
>
> Since v1: ...
>
> > vl.c | 6 --
> > 1 file changed, 4 insertions(+), 2 deletions(-)
> >
> > diff --git a/vl.c b/vl.c
> > index 4e25c78..69fc77c 100644
> > --- a/vl.c
> > +++ b/vl.c
> > @@ -2207,8 +2207,10 @@ static int parse_fw_cfg(void *opaque, QemuOpts
> *opts, Error **errp)
> > size = strlen(str); /* NUL terminator NOT included in fw_cfg
> blob */
> > buf = g_memdup(str, size);
> > } else {
> > -if (!g_file_get_contents(file, , , NULL)) {
> > -error_report("can't load %s", file);
> > +GError *error = NULL;
> > +if (!g_file_get_contents(file, , , )) {
> > +error_report("can't load %s, %s", file, error->message);
>
> If you ever respin, you can help Markus [1] effort using:
>
> error_setg(errp, "can't load %s, %s", file, error->message);
>
> Else your patch will clash with [2].
>
OK, I will send out this patch based Markus' tree or later when his patch
goes to upstream.
Thanks,
Li Qiang
>
> > +g_error_free(error);
> > return -1;
> > }
> > }
> >
>
> Regards,
>
> Phil.
>
> [1] https://lists.gnu.org/archive/html/qemu-devel/2018-10/msg01394.html
> [2] https://lists.gnu.org/archive/html/qemu-devel/2018-10/msg01406.html
>
[Qemu-devel] [PATCH] i386: pc_sysfw: load bios using rom API
As the bios is a ROM, just using rom API.
AFAICS no functionality changed.
Signed-off-by: Li Qiang
---
hw/i386/pc_sysfw.c | 10 +-
1 file changed, 5 insertions(+), 5 deletions(-)
diff --git a/hw/i386/pc_sysfw.c b/hw/i386/pc_sysfw.c
index 091e22dd60..7f469fd582 100644
--- a/hw/i386/pc_sysfw.c
+++ b/hw/i386/pc_sysfw.c
@@ -209,9 +209,9 @@ static void old_pc_system_rom_init(MemoryRegion
*rom_memory, bool isapc_ram_fw)
goto bios_error;
}
bios = g_malloc(sizeof(*bios));
-memory_region_init_ram(bios, NULL, "pc.bios", bios_size, _fatal);
-if (!isapc_ram_fw) {
-memory_region_set_readonly(bios, true);
+memory_region_init_rom(bios, NULL, "pc.bios", bios_size, _fatal);
+if (isapc_ram_fw) {
+memory_region_set_readonly(bios, false);
}
ret = rom_add_file_fixed(bios_name, (uint32_t)(-bios_size), -1);
if (ret != 0) {
@@ -230,8 +230,8 @@ static void old_pc_system_rom_init(MemoryRegion
*rom_memory, bool isapc_ram_fw)
0x10 - isa_bios_size,
isa_bios,
1);
-if (!isapc_ram_fw) {
-memory_region_set_readonly(isa_bios, true);
+if (isapc_ram_fw) {
+memory_region_set_readonly(isa_bios, false);
}
/* map all the bios at the top of memory */
--
2.11.0
Re: [Qemu-devel] [PATCH v2] vl.c: print error message if load fw_cfg file failed
Hi Li,
On 09/10/2018 04:39, Li Qiang wrote:
> It makes sense to print the error message while reading
> file failed.
OK
>
> Change since v1:
> free error
Changes are useful for reviewer, but not in the git history.
You can have them automatically stripped if you place them below the
next '---' separator.
Hopefully the maintainer taking this patch can clean this up.
>
> Signed-off-by: Li Qiang
> Reviewed-by: Philippe Mathieu-Daudé
> ---
Here goes comment not worth to stay forever in git history:
Since v1: ...
> vl.c | 6 --
> 1 file changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/vl.c b/vl.c
> index 4e25c78..69fc77c 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -2207,8 +2207,10 @@ static int parse_fw_cfg(void *opaque, QemuOpts *opts,
> Error **errp)
> size = strlen(str); /* NUL terminator NOT included in fw_cfg blob */
> buf = g_memdup(str, size);
> } else {
> -if (!g_file_get_contents(file, , , NULL)) {
> -error_report("can't load %s", file);
> +GError *error = NULL;
> +if (!g_file_get_contents(file, , , )) {
> +error_report("can't load %s, %s", file, error->message);
If you ever respin, you can help Markus [1] effort using:
error_setg(errp, "can't load %s, %s", file, error->message);
Else your patch will clash with [2].
> +g_error_free(error);
> return -1;
> }
> }
>
Regards,
Phil.
[1] https://lists.gnu.org/archive/html/qemu-devel/2018-10/msg01394.html
[2] https://lists.gnu.org/archive/html/qemu-devel/2018-10/msg01406.html
[Qemu-devel] [PATCH v2 3/3] Travis support for the acceptance tests
This enables the execution of the acceptance tests on Travis. Because the Travis environment is based on Ubuntu Trusty, it requires the python3-pip. Note: while another supposedely required component on newer versions (such as on Bionic) split the Python 3 installation further on the python3-venv package. Signed-off-by: Cleber Rosa --- .travis.yml | 6 ++ 1 file changed, 6 insertions(+) diff --git a/.travis.yml b/.travis.yml index 95be6ec59f..db1a31ea51 100644 --- a/.travis.yml +++ b/.travis.yml @@ -36,6 +36,7 @@ addons: - liburcu-dev - libusb-1.0-0-dev - libvte-2.90-dev + - python3-pip - sparse - uuid-dev - gcovr @@ -117,6 +118,11 @@ matrix: - env: CONFIG="--target-list=x86_64-softmmu" python: - "3.6" +# Acceptance (Functional) tests +- env: CONFIG="--python=/usr/bin/python3 --target-list=x86_64-softmmu" + TEST_CMD="make check-acceptance" + python: +- "3.6" # Using newer GCC with sanitizers - addons: apt: -- 2.17.1
[Qemu-devel] [PATCH v2 2/3] Acceptance tests: add make rule for running them
The acceptance (aka functional, aka Avocado-based) tests are Python files located in "tests/acceptance" that need to be run with the Avocado libs and test runner. Let's provide a convenient way for QEMU developers to run them, by making use of the tests-venv with the required setup. Also, while the Avocado test runner will take care of creating a location to save test results to, it was understood that it's better if the results are kept within the build tree. Signed-off-by: Cleber Rosa --- docs/devel/testing.rst | 35 ++- tests/Makefile.include | 17 +++-- tests/venv-requirements.txt | 1 + 3 files changed, 46 insertions(+), 7 deletions(-) diff --git a/docs/devel/testing.rst b/docs/devel/testing.rst index 727c4019b5..b992a2961d 100644 --- a/docs/devel/testing.rst +++ b/docs/devel/testing.rst @@ -545,10 +545,31 @@ Tests based on ``avocado_qemu.Test`` can easily: - http://avocado-framework.readthedocs.io/en/latest/api/test/avocado.html#avocado.Test - http://avocado-framework.readthedocs.io/en/latest/api/utils/avocado.utils.html -Installation - +Running tests +- -To install Avocado and its dependencies, run: +You can run the acceptance tests simply by executing: + +.. code:: + + make check-acceptance + +This involves the automatic creation of Python virtual environment +within the build tree (at ``tests/venv``) which will have all the +right dependencies, and will save tests results also within the +build tree (at ``tests/results``). + +Note: the build environment must be using a Python 3 stack, and have +the ``venv`` and ``pip`` packages installed. If necessary, make sure +``configure`` is called with ``--python=`` and that those modules are +available. On Debian and Ubuntu based systems, depending on the +specific version, they may be on packages named ``python3-venv`` and +``python3-pip``. + +Manual Installation +--- + +To manually install Avocado and its dependencies, run: .. code:: @@ -689,11 +710,15 @@ The exact QEMU binary to be used on QEMUMachine. Uninstalling Avocado -If you've followed the installation instructions above, you can easily -uninstall Avocado. Start by listing the packages you have installed:: +If you've followed the manual installation instructions above, you can +easily uninstall Avocado. Start by listing the packages you have +installed:: pip list --user And remove any package you want with:: pip uninstall + +If you've used ``make check-acceptance``, the Python virtual environment where +Avocado is installed will be cleaned up as part of ``make check-clean``. diff --git a/tests/Makefile.include b/tests/Makefile.include index 68af79927d..00fdf9913e 100644 --- a/tests/Makefile.include +++ b/tests/Makefile.include @@ -11,6 +11,7 @@ check-help: @echo " $(MAKE) check-qapi-schemaRun QAPI schema tests" @echo " $(MAKE) check-block Run block tests" @echo " $(MAKE) check-tcgRun TCG tests" + @echo " $(MAKE) check-acceptance Run all acceptance (functional) tests" @echo " $(MAKE) check-report.htmlGenerates an HTML test report" @echo " $(MAKE) check-venv Creates a Python venv for tests" @echo " $(MAKE) check-clean Clean the tests" @@ -1018,10 +1019,11 @@ check-decodetree: # Python venv for running tests -.PHONY: check-venv +.PHONY: check-venv check-acceptance TESTS_VENV_DIR=$(BUILD_DIR)/tests/venv TESTS_VENV_REQ=$(SRC_PATH)/tests/venv-requirements.txt +TESTS_RESULTS_DIR=$(BUILD_DIR)/tests/results $(TESTS_VENV_DIR): $(call quiet-command, \ @@ -1031,8 +1033,19 @@ $(TESTS_VENV_DIR): $(TESTS_VENV_DIR)/bin/pip -q install -r $(TESTS_VENV_REQ), \ PIP, $(TESTS_VENV_REQ)) +$(TESTS_RESULTS_DIR): + $(call quiet-command, mkdir -p $@, \ +MKDIR, $@) + check-venv: $(TESTS_VENV_DIR) +check-acceptance: check-venv $(TESTS_RESULTS_DIR) + $(call quiet-command, \ +$(TESTS_VENV_DIR)/bin/python -m avocado \ +--show=none run --job-results-dir=$(TESTS_RESULTS_DIR) --failfast=on \ +$(SRC_PATH)/tests/acceptance, \ +"AVOCADO", "tests/acceptance") + # Consolidated targets .PHONY: check-qapi-schema check-qtest check-unit check check-clean @@ -1046,7 +1059,7 @@ check-clean: rm -rf $(check-unit-y) tests/*.o $(QEMU_IOTESTS_HELPERS-y) rm -rf $(sort $(foreach target,$(SYSEMU_TARGET_LIST), $(check-qtest-$(target)-y)) $(check-qtest-generic-y)) rm -f tests/test-qapi-gen-timestamp - rm -rf $(TESTS_VENV_DIR) + rm -rf $(TESTS_VENV_DIR) $(TESTS_RESULTS_DIR) clean: check-clean diff --git a/tests/venv-requirements.txt b/tests/venv-requirements.txt index d39f9d1576..64c6e27a94 100644 --- a/tests/venv-requirements.txt +++ b/tests/venv-requirements.txt @@ -1,3 +1,4 @@ # Add Python module requirements, one
[Qemu-devel] [PATCH v2 0/3] Bootstrap Python venv and acceptance/functional tests
TL;DR = Allow acceptance tests to be run with `make check-acceptance`. Details === This introduces a Python virtual environment that will be setup within the QEMU build directory, that will contain the exact environment that tests may require. There's one current caveat: it requires Python 3, as it's based on the venv module. This was based on some discussions and perception about standardizing on Python 3, but can easily be made to accommodate Python 2 as well. Changes from v1: * TESTS_VENV_REQ (the path of "venv-requirements.txt") now points to the source path ($SRC_PATH instead of $BUILD_DIR) * Create the venv with "--system-site-packages", which allows the reuse of packages (and no additional downloads) in case there's a package installed system wide providing the same package and version. * Run Avocado with "python -m avocado". It may have been installed reusing the system wide packages, and then the script may not be available on the venv. * Improved documentation describing the Python 3, venv and pip requirements. * Updated avocado-framework requirement to latest released version (65.0) * (New commit) Added support for running the acceptance tests on Travis. Ideas discussed, but not implemented: * Install external packages such as python3-pip on Debian based systems, deemed too invasive on developer's systems. * Allow the use of Python 2, and consequently the "virtualenv" module. Cleber Rosa (3): Bootstrap Python venv for tests Acceptance tests: add make rule for running them Travis support for the acceptance tests .travis.yml | 6 ++ docs/devel/testing.rst | 35 ++- tests/Makefile.include | 32 tests/venv-requirements.txt | 4 4 files changed, 72 insertions(+), 5 deletions(-) create mode 100644 tests/venv-requirements.txt -- 2.17.1
[Qemu-devel] [PATCH v2 1/3] Bootstrap Python venv for tests
A number of QEMU tests are written in Python, and may benefit from an untainted Python venv. By using make rules, tests that depend on specific Python libs can set that rule as a requiment, along with rules that require the presence or installation of specific libraries. The tests/venv-requirements.txt is supposed to contain the Python requirements that should be added to the venv created by check-venv. Signed-off-by: Cleber Rosa --- tests/Makefile.include | 19 +++ tests/venv-requirements.txt | 3 +++ 2 files changed, 22 insertions(+) create mode 100644 tests/venv-requirements.txt diff --git a/tests/Makefile.include b/tests/Makefile.include index 7a3059bf6c..68af79927d 100644 --- a/tests/Makefile.include +++ b/tests/Makefile.include @@ -12,6 +12,7 @@ check-help: @echo " $(MAKE) check-block Run block tests" @echo " $(MAKE) check-tcgRun TCG tests" @echo " $(MAKE) check-report.htmlGenerates an HTML test report" + @echo " $(MAKE) check-venv Creates a Python venv for tests" @echo " $(MAKE) check-clean Clean the tests" @echo @echo "Please note that HTML reports do not regenerate if the unit tests" @@ -1015,6 +1016,23 @@ check-decodetree: ./check.sh "$(PYTHON)" "$(SRC_PATH)/scripts/decodetree.py", \ TEST, decodetree.py) +# Python venv for running tests + +.PHONY: check-venv + +TESTS_VENV_DIR=$(BUILD_DIR)/tests/venv +TESTS_VENV_REQ=$(SRC_PATH)/tests/venv-requirements.txt + +$(TESTS_VENV_DIR): + $(call quiet-command, \ +$(PYTHON) -m venv --system-site-packages $@, \ +VENV, $@) + $(call quiet-command, \ +$(TESTS_VENV_DIR)/bin/pip -q install -r $(TESTS_VENV_REQ), \ +PIP, $(TESTS_VENV_REQ)) + +check-venv: $(TESTS_VENV_DIR) + # Consolidated targets .PHONY: check-qapi-schema check-qtest check-unit check check-clean @@ -1028,6 +1046,7 @@ check-clean: rm -rf $(check-unit-y) tests/*.o $(QEMU_IOTESTS_HELPERS-y) rm -rf $(sort $(foreach target,$(SYSEMU_TARGET_LIST), $(check-qtest-$(target)-y)) $(check-qtest-generic-y)) rm -f tests/test-qapi-gen-timestamp + rm -rf $(TESTS_VENV_DIR) clean: check-clean diff --git a/tests/venv-requirements.txt b/tests/venv-requirements.txt new file mode 100644 index 00..d39f9d1576 --- /dev/null +++ b/tests/venv-requirements.txt @@ -0,0 +1,3 @@ +# Add Python module requirements, one per line, to be installed +# in the tests/venv Python virtual environment. For more info, +# refer to: https://pip.pypa.io/en/stable/user_guide/#id1 -- 2.17.1
Re: [Qemu-devel] [PATCH 09/31] ioapic: Fix error handling in realize()
On Mon, Oct 08, 2018 at 07:31:03PM +0200, Markus Armbruster wrote: > Calling error_report() in a function that takes an Error ** argument > is suspicious. ioapic_realize() does that, and then exit()s. > Currently mostly harmless, as the device cannot be hot-plugged. > > Fixes: 20fd4b7b6d9282fe0cb83601f1821f31bd257458 > Cc: Peter Xu > Signed-off-by: Markus Armbruster Reviewed-by: Peter Xu -- Peter Xu
[Qemu-devel] [PATCH v2] vl.c: print error message if load fw_cfg file failed
It makes sense to print the error message while reading
file failed.
Change since v1:
free error
Signed-off-by: Li Qiang
Reviewed-by: Philippe Mathieu-Daud??
---
vl.c | 6 --
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/vl.c b/vl.c
index 4e25c78..69fc77c 100644
--- a/vl.c
+++ b/vl.c
@@ -2207,8 +2207,10 @@ static int parse_fw_cfg(void *opaque, QemuOpts *opts,
Error **errp)
size = strlen(str); /* NUL terminator NOT included in fw_cfg blob */
buf = g_memdup(str, size);
} else {
-if (!g_file_get_contents(file, , , NULL)) {
-error_report("can't load %s", file);
+GError *error = NULL;
+if (!g_file_get_contents(file, , , )) {
+error_report("can't load %s, %s", file, error->message);
+g_error_free(error);
return -1;
}
}
--
1.8.3.1
Re: [Qemu-devel] [PATCH] util: aio-posix: fix a typo
On Sun, 10/07 19:16, Li Qiang wrote: > Cc: qemu-triv...@nongnu.org > Signed-off-by: Li Qiang Reviewed-by: Fam Zheng
Re: [Qemu-devel] [PATCH 03/31] cpus hw target: Use warn_report() & friends to report warnings
On Mon, Oct 08, 2018 at 07:30:57PM +0200, Markus Armbruster wrote:
> Calling error_report() in a function that takes an Error ** argument
> is suspicious. Convert a few that are actually warnings to
> warn_report().
>
> While there, split a warning consisting of multiple sentences to
> conform to conventions spelled out in warn_report()'s contract.
>
> Cc: Alex Bennée
> Cc: Mark Cave-Ayland
> Cc: Alex Williamson
> Cc: Fam Zheng
> Cc: Wei Huang
> Cc: David Gibson
> Signed-off-by: Markus Armbruster
ppc part
Acked-by: David Gibson
> ---
> cpus.c | 8
> hw/display/cg3.c| 2 +-
> hw/display/tcx.c| 2 +-
> hw/misc/ivshmem.c | 4 ++--
> hw/net/virtio-net.c | 8
> hw/virtio/virtio-pci.c | 4 ++--
> target/i386/cpu.c | 17 +
> target/ppc/translate_init.inc.c | 4 ++--
> 8 files changed, 25 insertions(+), 24 deletions(-)
>
> diff --git a/cpus.c b/cpus.c
> index 361678e459..7804071872 100644
> --- a/cpus.c
> +++ b/cpus.c
> @@ -211,12 +211,12 @@ void qemu_tcg_configure(QemuOpts *opts, Error **errp)
> error_setg(errp, "No MTTCG when icount is enabled");
> } else {
> #ifndef TARGET_SUPPORTS_MTTCG
> -error_report("Guest not yet converted to MTTCG - "
> - "you may get unexpected results");
> +warn_report("Guest not yet converted to MTTCG - "
> +"you may get unexpected results");
> #endif
> if (!check_tcg_memory_orders_compatible()) {
> -error_report("Guest expects a stronger memory ordering "
> - "than the host provides");
> +warn_report("Guest expects a stronger memory ordering "
> +"than the host provides");
> error_printf("This may cause strange/hard to debug
> errors\n");
> }
> mttcg_enabled = true;
> diff --git a/hw/display/cg3.c b/hw/display/cg3.c
> index 1c199ab369..e50d97e48c 100644
> --- a/hw/display/cg3.c
> +++ b/hw/display/cg3.c
> @@ -307,7 +307,7 @@ static void cg3_realizefn(DeviceState *dev, Error **errp)
> ret = load_image_mr(fcode_filename, >rom);
> g_free(fcode_filename);
> if (ret < 0 || ret > FCODE_MAX_ROM_SIZE) {
> -error_report("cg3: could not load prom '%s'", CG3_ROM_FILE);
> +warn_report("cg3: could not load prom '%s'", CG3_ROM_FILE);
> }
> }
>
> diff --git a/hw/display/tcx.c b/hw/display/tcx.c
> index b2786ee8d0..66f2459226 100644
> --- a/hw/display/tcx.c
> +++ b/hw/display/tcx.c
> @@ -823,7 +823,7 @@ static void tcx_realizefn(DeviceState *dev, Error **errp)
> ret = load_image_mr(fcode_filename, >rom);
> g_free(fcode_filename);
> if (ret < 0 || ret > FCODE_MAX_ROM_SIZE) {
> -error_report("tcx: could not load prom '%s'", TCX_ROM_FILE);
> +warn_report("tcx: could not load prom '%s'", TCX_ROM_FILE);
> }
> }
>
> diff --git a/hw/misc/ivshmem.c b/hw/misc/ivshmem.c
> index 6febbabcaa..4794518b2c 100644
> --- a/hw/misc/ivshmem.c
> +++ b/hw/misc/ivshmem.c
> @@ -1292,8 +1292,8 @@ static void ivshmem_realize(PCIDevice *dev, Error
> **errp)
> IVShmemState *s = IVSHMEM_COMMON(dev);
>
> if (!qtest_enabled()) {
> -error_report("ivshmem is deprecated, please use ivshmem-plain"
> - " or ivshmem-doorbell instead");
> +warn_report("ivshmem is deprecated, please use ivshmem-plain"
> +" or ivshmem-doorbell instead");
> }
>
> if (qemu_chr_fe_backend_connected(>server_chr) + !!s->shmobj != 1) {
> diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
> index 4bdd5b8532..385b1a03e9 100644
> --- a/hw/net/virtio-net.c
> +++ b/hw/net/virtio-net.c
> @@ -2020,10 +2020,10 @@ static void virtio_net_device_realize(DeviceState
> *dev, Error **errp)
>
> if (n->net_conf.tx && strcmp(n->net_conf.tx, "timer")
> && strcmp(n->net_conf.tx, "bh")) {
> -error_report("virtio-net: "
> - "Unknown option tx=%s, valid options: \"timer\" \"bh\"",
> - n->net_conf.tx);
> -error_report("Defaulting to \"bh\"");
> +warn_report("virtio-net: "
> +"Unknown option tx=%s, valid options: \"timer\" \"bh\"",
> +n->net_conf.tx);
> +error_printf("Defaulting to \"bh\"");
> }
>
> n->net_conf.tx_queue_size = MIN(virtio_net_max_tx_queue_size(n),
> diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
> index 3a01fe90f0..a954799267 100644
> --- a/hw/virtio/virtio-pci.c
> +++ b/hw/virtio/virtio-pci.c
> @@ -1683,8 +1683,8 @@ static void virtio_pci_device_plugged(DeviceState *d,
> Error **errp)
> if (err) {
> /* Notice when
Re: [Qemu-devel] [PATCH 01/31] Use error_fatal to simplify obvious fatal errors (again)
On Mon, Oct 08, 2018 at 07:30:55PM +0200, Markus Armbruster wrote:
> Add a slight improvement of the Coccinelle semantic patch from commit
> 07d04a0219b, and use it to clean up. It leaves dead Error * variables
> behind, cleaned up manually.
>
> Cc: David Gibson
> Cc: Alexander Graf
> Cc: Eric Blake
> Cc: Paolo Bonzini
> Signed-off-by: Markus Armbruster
ppc part
Acked-by: David Gibson
> ---
> hw/intc/xics_kvm.c | 7 +--
> qemu-nbd.c | 6 +-
> scripts/coccinelle/use-error_fatal.cocci | 20
> vl.c | 7 +--
> 4 files changed, 23 insertions(+), 17 deletions(-)
> create mode 100644 scripts/coccinelle/use-error_fatal.cocci
>
> diff --git a/hw/intc/xics_kvm.c b/hw/intc/xics_kvm.c
> index 30c3769a20..e8fa9a53ae 100644
> --- a/hw/intc/xics_kvm.c
> +++ b/hw/intc/xics_kvm.c
> @@ -198,17 +198,12 @@ static void ics_get_kvm_state(ICSState *ics)
> {
> uint64_t state;
> int i;
> -Error *local_err = NULL;
>
> for (i = 0; i < ics->nr_irqs; i++) {
> ICSIRQState *irq = >irqs[i];
>
> kvm_device_access(kernel_xics_fd, KVM_DEV_XICS_GRP_SOURCES,
> - i + ics->offset, , false, _err);
> -if (local_err) {
> -error_report_err(local_err);
> -exit(1);
> -}
> + i + ics->offset, , false, _fatal);
>
> irq->server = state & KVM_XICS_DESTINATION_MASK;
> irq->saved_priority = (state >> KVM_XICS_PRIORITY_SHIFT)
> diff --git a/qemu-nbd.c b/qemu-nbd.c
> index e76fe3082a..7874bc973c 100644
> --- a/qemu-nbd.c
> +++ b/qemu-nbd.c
> @@ -1002,11 +1002,7 @@ int main(int argc, char **argv)
> }
>
> exp = nbd_export_new(bs, dev_offset, fd_size, nbdflags,
> nbd_export_closed,
> - writethrough, NULL, _err);
> -if (!exp) {
> -error_report_err(local_err);
> -exit(EXIT_FAILURE);
> -}
> + writethrough, NULL, _fatal);
> nbd_export_set_name(exp, export_name);
> nbd_export_set_description(exp, export_description);
>
> diff --git a/scripts/coccinelle/use-error_fatal.cocci
> b/scripts/coccinelle/use-error_fatal.cocci
> new file mode 100644
> index 00..10fff0aec4
> --- /dev/null
> +++ b/scripts/coccinelle/use-error_fatal.cocci
> @@ -0,0 +1,20 @@
> +@@
> +type T;
> +identifier FUN, RET;
> +expression list ARGS;
> +expression ERR, EC, FAIL;
> +@@
> +(
> +-T RET = FUN(ARGS, );
> ++T RET = FUN(ARGS, _fatal);
> +|
> +-RET = FUN(ARGS, );
> ++RET = FUN(ARGS, _fatal);
> +|
> +-FUN(ARGS, );
> ++FUN(ARGS, _fatal);
> +)
> +-if (FAIL) {
> +-error_report_err(ERR);
> +-exit(EC);
> +-}
> diff --git a/vl.c b/vl.c
> index a867c9c4d9..9d2b38a31f 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -2002,15 +2002,10 @@ static void select_vgahw(const char *p)
>
> static void parse_display_qapi(const char *optarg)
> {
> -Error *err = NULL;
> DisplayOptions *opts;
> Visitor *v;
>
> -v = qobject_input_visitor_new_str(optarg, "type", );
> -if (!v) {
> -error_report_err(err);
> -exit(1);
> -}
> +v = qobject_input_visitor_new_str(optarg, "type", _fatal);
>
> visit_type_DisplayOptions(v, NULL, , _fatal);
> QAPI_CLONE_MEMBERS(DisplayOptions, , opts);
--
David Gibson| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson
signature.asc
Description: PGP signature
[Qemu-devel] [Bug 1795527] Re: Malformed audio and video output stuttering after upgrade to QEMU 3.0
UPDATE 2 (STUTTERING ELIMINATED, AUDIO ISSUES _STILL_ PRESENT) I think I've tracked down the source of the stuttering that affected my machine, and it doesn't seem to be QEMU-related. I'm going to write something about it here anyway, waiting to report it to other, more appropriate channels, hoping that it could help someone else out there. QUICK FIX: toggle the `kvm=off` flag off, perform a boot/shutdown cycle, toggle it back on, boot again. POSSIBLE EXPLANATION: During the week of panicking trial troubleshooting between the discovery of the audio issue and this report's filling, I let Windows perform an update. It downloaded and installed KB4100347. Now, by the looks of things this update naively checks the MSR for hipervisor signatures and, not finding any because of the KVM hidden state flag, it carries on with its duties of putting in place whatever microcode-powered mitigations for Spectre V2 it is instructed to set up. Being the CPU really a virtual one, the process, unsurprisingly, fails, and the OS is left with some sort of software gimmick which tries to compensate for those unsupported functions. To my surprise, it seems that the MSR check is implemented as a persistent trigger and, as soon as it detects an hypervisor signature, it permanently removes the mitigation routines, leaving a smoothly running VM. I've performed 3 reboots since I toggled the flag off and on again, and the performance are consistent. Obviously, this explanation is just a wild guess, but I find it believable enough and in line with my experience. As I stated in the title, though, *the audio corruption is still well present* under QEMU 3.0, and eliminating the stuttering didn't help, leaving me with the original and most important problem. -- You received this bug notification because you are a member of qemu- devel-ml, which is subscribed to QEMU. https://bugs.launchpad.net/bugs/1795527 Title: Malformed audio and video output stuttering after upgrade to QEMU 3.0 Status in QEMU: New Bug description: My host is an x86_64 Arch Linux OS with a recompiled 4.18.10 hardened kernel, running a few KVM guests with varying OSes and configurations managed through a Libvirt stack. Among these guests I have two Windows 10 VMs with VGA passthrough and PulseAudio-backed virtual audio devices. After upgrading to QEMU 3.0.0, both of the Win10 guests started showing corrupted audio output in the form of unnatural reproduction speed and occasional but consistently misplaced audio fragments originating from what seems to be a circular buffer wrapping over itself (misbehaviour detected by starting some games with known OSTs and dialogues: soundtracks sound accelerated and past dialogue lines start replaying middle-sentence until the next line starts playing). In addition, the video output of the malfunctioning VMs regularly stutters roughly twice a second for a fraction of a second (sync'ed with the suspected buffer wrapping and especially pronounced during not-pre-rendered cutscenes), toghether with mouse freezes that look like actual input misses more than simple lack of screen refreshes. The issue was succesfully reproduced without the managing stack, directly with the following command line, on the most capable Windows guest: QEMU_AUDIO_DRV=pa QEMU_PA_SERVER=127.0.0.1 /usr/bin/qemu-system-x86_64 -name guest=win10_gms,debug-threads=on \ -machine pc-i440fx-3.0,accel=kvm,usb=off,vmport=off,dump-guest-core=off \ -cpu host,hv_time,hv_relaxed,hv_vapic,hv_spinlocks=0x1fff,hv_vendor_id=123456789abc,kvm=off \ -drive file=/usr/share/ovmf/x64/OVMF_CODE.fd,if=pflash,format=raw,unit=0,readonly=on \ -drive file=/var/lib/libvirt/qemu/nvram/win10_gms_VARS.fd,if=pflash,format=raw,unit=1 \ -m 5120 \ -realtime mlock=off \ -smp 3,sockets=1,cores=3,threads=1 \ -uuid 39b56ee2-6bae-4009-9108-7be26d5d63ac \ -display none \ -no-user-config \ -nodefaults \ -rtc base=localtime,driftfix=slew \ -global kvm-pit.lost_tick_policy=delay \ -no-hpet \ -no-shutdown \ -global PIIX4_PM.disable_s3=1 \ -global PIIX4_PM.disable_s4=1 \ -boot strict=on \ -device ich9-usb-ehci1,id=usb,bus=pci.0,addr=0x4.0x7 \ -device ich9-usb-uhci1,masterbus=usb.0,firstport=0,bus=pci.0,multifunction=on,addr=0x4 \ -device ich9-usb-uhci2,masterbus=usb.0,firstport=2,bus=pci.0,addr=0x4.0x1 \
[Qemu-devel] [RFC v2 5/5] cputlb: dynamically resize TLBs based on use rate
Perform the resizing only on flushes, otherwise we'd have to take a perf hit by either rehashing the array or unnecessarily flushing it. We grow the array aggressively, and reduce the size more slowly. This accommodates mixed workloads, where some processes might be memory-heavy while others are not. As the following experiments show, this a net perf gain, particularly for memory-heavy workloads. Experiments are run on an Intel i7-6700K CPU @ 4.00GHz. 1. System boot + shudown, debian aarch64: - Before (tb-lock-v3): Performance counter stats for 'taskset -c 0 ../img/aarch64/die.sh' (10 runs): 7469.363393 task-clock (msec) #0.998 CPUs utilized ( +- 0.07% ) 31,507,707,190 cycles#4.218 GHz ( +- 0.07% ) 57,101,577,452 instructions #1.81 insns per cycle ( +- 0.08% ) 10,265,531,804 branches # 1374.352 M/sec ( +- 0.07% ) 173,020,681 branch-misses #1.69% of all branches ( +- 0.10% ) 7.483359063 seconds time elapsed ( +- 0.08% ) - After: Performance counter stats for 'taskset -c 0 ../img/aarch64/die.sh' (10 runs): 7185.036730 task-clock (msec) #0.999 CPUs utilized ( +- 0.11% ) 30,303,501,143 cycles#4.218 GHz ( +- 0.11% ) 54,198,386,487 instructions #1.79 insns per cycle ( +- 0.08% ) 9,726,518,945 branches # 1353.719 M/sec ( +- 0.08% ) 167,082,307 branch-misses #1.72% of all branches ( +- 0.08% ) 7.195597842 seconds time elapsed ( +- 0.11% ) That is, a 3.8% improvement. 2. System boot + shutdown, ubuntu 18.04 x86_64: - Before (tb-lock-v3): Performance counter stats for 'taskset -c 0 ../img/x86_64/ubuntu-die.sh -nographic' (2 runs): 49971.036482 task-clock (msec) #0.999 CPUs utilized ( +- 1.62% ) 210,766,077,140 cycles#4.218 GHz ( +- 1.63% ) 428,829,830,790 instructions #2.03 insns per cycle ( +- 0.75% ) 77,313,384,038 branches # 1547.164 M/sec ( +- 0.54% ) 835,610,706 branch-misses #1.08% of all branches ( +- 2.97% ) 50.003855102 seconds time elapsed ( +- 1.61% ) - After: Performance counter stats for 'taskset -c 0 ../img/x86_64/ubuntu-die.sh -nographic' (2 runs): 50118.124477 task-clock (msec) #0.999 CPUs utilized ( +- 4.30% ) 132,396 context-switches #0.003 M/sec ( +- 1.20% ) 0 cpu-migrations#0.000 K/sec ( +-100.00% ) 167,754 page-faults #0.003 M/sec ( +- 0.06% ) 211,414,701,601 cycles#4.218 GHz ( +- 4.30% ) stalled-cycles-frontend stalled-cycles-backend 431,618,818,597 instructions #2.04 insns per cycle ( +- 6.40% ) 80,197,256,524 branches # 1600.165 M/sec ( +- 8.59% ) 794,830,352 branch-misses #0.99% of all branches ( +- 2.05% ) 50.177077175 seconds time elapsed ( +- 4.23% ) No improvement (within noise range). 3. x86_64 SPEC06int: SPEC06int (test set) [ Y axis: speedup over master ] 8 +-+--+++-+++++++-+++--+-+ | | | tlb-lock-v3 | 7 +-+..$$$...+indirection +-+ |$ $ +resizing | |$ $| 6 +-+..$.$..+-+ |$ $| |$ $| 5 +-+..$.$..+-+ |$ $| |$ $| 4 +-+..$.$..+-+ |$ $
[Qemu-devel] [RFC v2 1/5] tcg: Add tlb_index and tlb_entry helpers
From: Richard Henderson
Isolate the computation of an index from an address into a
helper before we change that function.
Signed-off-by: Richard Henderson
[ cota: convert tlb_vaddr_to_host; use atomic_read on addr_write ]
Signed-off-by: Emilio G. Cota
---
accel/tcg/softmmu_template.h | 72
include/exec/cpu_ldst.h | 19 +++--
include/exec/cpu_ldst_template.h | 25 +--
accel/tcg/cputlb.c | 61 ---
4 files changed, 92 insertions(+), 85 deletions(-)
diff --git a/accel/tcg/softmmu_template.h b/accel/tcg/softmmu_template.h
index 1e50263871..aa34c692ee 100644
--- a/accel/tcg/softmmu_template.h
+++ b/accel/tcg/softmmu_template.h
@@ -111,9 +111,10 @@ static inline DATA_TYPE glue(io_read, SUFFIX)(CPUArchState
*env,
WORD_TYPE helper_le_ld_name(CPUArchState *env, target_ulong addr,
TCGMemOpIdx oi, uintptr_t retaddr)
{
-unsigned mmu_idx = get_mmuidx(oi);
-int index = (addr >> TARGET_PAGE_BITS) & (CPU_TLB_SIZE - 1);
-target_ulong tlb_addr = env->tlb_table[mmu_idx][index].ADDR_READ;
+uintptr_t mmu_idx = get_mmuidx(oi);
+uintptr_t index = tlb_index(env, mmu_idx, addr);
+CPUTLBEntry *entry = tlb_entry(env, mmu_idx, addr);
+target_ulong tlb_addr = entry->ADDR_READ;
unsigned a_bits = get_alignment_bits(get_memop(oi));
uintptr_t haddr;
DATA_TYPE res;
@@ -129,7 +130,7 @@ WORD_TYPE helper_le_ld_name(CPUArchState *env, target_ulong
addr,
tlb_fill(ENV_GET_CPU(env), addr, DATA_SIZE, READ_ACCESS_TYPE,
mmu_idx, retaddr);
}
-tlb_addr = env->tlb_table[mmu_idx][index].ADDR_READ;
+tlb_addr = entry->ADDR_READ;
}
/* Handle an IO access. */
@@ -166,7 +167,7 @@ WORD_TYPE helper_le_ld_name(CPUArchState *env, target_ulong
addr,
return res;
}
-haddr = addr + env->tlb_table[mmu_idx][index].addend;
+haddr = addr + entry->addend;
#if DATA_SIZE == 1
res = glue(glue(ld, LSUFFIX), _p)((uint8_t *)haddr);
#else
@@ -179,9 +180,10 @@ WORD_TYPE helper_le_ld_name(CPUArchState *env,
target_ulong addr,
WORD_TYPE helper_be_ld_name(CPUArchState *env, target_ulong addr,
TCGMemOpIdx oi, uintptr_t retaddr)
{
-unsigned mmu_idx = get_mmuidx(oi);
-int index = (addr >> TARGET_PAGE_BITS) & (CPU_TLB_SIZE - 1);
-target_ulong tlb_addr = env->tlb_table[mmu_idx][index].ADDR_READ;
+uintptr_t mmu_idx = get_mmuidx(oi);
+uintptr_t index = tlb_index(env, mmu_idx, addr);
+CPUTLBEntry *entry = tlb_entry(env, mmu_idx, addr);
+target_ulong tlb_addr = entry->ADDR_READ;
unsigned a_bits = get_alignment_bits(get_memop(oi));
uintptr_t haddr;
DATA_TYPE res;
@@ -197,7 +199,7 @@ WORD_TYPE helper_be_ld_name(CPUArchState *env, target_ulong
addr,
tlb_fill(ENV_GET_CPU(env), addr, DATA_SIZE, READ_ACCESS_TYPE,
mmu_idx, retaddr);
}
-tlb_addr = env->tlb_table[mmu_idx][index].ADDR_READ;
+tlb_addr = entry->ADDR_READ;
}
/* Handle an IO access. */
@@ -234,7 +236,7 @@ WORD_TYPE helper_be_ld_name(CPUArchState *env, target_ulong
addr,
return res;
}
-haddr = addr + env->tlb_table[mmu_idx][index].addend;
+haddr = addr + entry->addend;
res = glue(glue(ld, LSUFFIX), _be_p)((uint8_t *)haddr);
return res;
}
@@ -275,10 +277,10 @@ static inline void glue(io_write, SUFFIX)(CPUArchState
*env,
void helper_le_st_name(CPUArchState *env, target_ulong addr, DATA_TYPE val,
TCGMemOpIdx oi, uintptr_t retaddr)
{
-unsigned mmu_idx = get_mmuidx(oi);
-int index = (addr >> TARGET_PAGE_BITS) & (CPU_TLB_SIZE - 1);
-target_ulong tlb_addr =
-atomic_read(>tlb_table[mmu_idx][index].addr_write);
+uintptr_t mmu_idx = get_mmuidx(oi);
+uintptr_t index = tlb_index(env, mmu_idx, addr);
+CPUTLBEntry *entry = tlb_entry(env, mmu_idx, addr);
+target_ulong tlb_addr = atomic_read(>addr_write);
unsigned a_bits = get_alignment_bits(get_memop(oi));
uintptr_t haddr;
@@ -293,8 +295,7 @@ void helper_le_st_name(CPUArchState *env, target_ulong
addr, DATA_TYPE val,
tlb_fill(ENV_GET_CPU(env), addr, DATA_SIZE, MMU_DATA_STORE,
mmu_idx, retaddr);
}
-tlb_addr = atomic_read(>tlb_table[mmu_idx][index].addr_write) &
-~TLB_INVALID_MASK;
+tlb_addr = atomic_read(>addr_write) & ~TLB_INVALID_MASK;
}
/* Handle an IO access. */
@@ -315,16 +316,16 @@ void helper_le_st_name(CPUArchState *env, target_ulong
addr, DATA_TYPE val,
if (DATA_SIZE > 1
&& unlikely((addr & ~TARGET_PAGE_MASK) + DATA_SIZE - 1
>= TARGET_PAGE_SIZE)) {
-int i, index2;
-target_ulong page2, tlb_addr2;
+int i;
+target_ulong page2;
+CPUTLBEntry *entry2;
do_unaligned_access:
/* Ensure the second
[Qemu-devel] [RFC v2 0/5] Dynamic TLB sizing
v1: https://lists.gnu.org/archive/html/qemu-devel/2018-10/msg01146.html Changes since v1: - Add tlb_index and tlb_entry helpers from Richard - Introduce sizeof_tlb() and tlb_n_entries() - Extract tlb_mask as its own array in CPUArchState, as suggested by Richard. For the associated helpers (tlb_index etc) I tried several approaches, and performance-wise they're all the same, so went for the simplest implementation. - Use uintptr_t for tlb_mask, as done in Richard's patch + tcg/i386: use hrexw when reading tlb_mask + Define tlbtype and tlbrexw solely based on TARGET_PAGE_BITS - Rename tlb_is_invalid to tlb_entry_is_empty, comparing all fields (except .addend) against -1. - Rename CPUTLBDesc.used to .n_used_entries. - Drop the MIN/MAX CPU_TLB_BITS patches, defining instead some values for MIN/MAX as well as a default. - Use new_size and old_size consistently in the resizing function, as suggested by Richard. - Add an additional chart to the last patch, where softmmu performance is compared against user-mode: https://imgur.com/a/eXkjMCE You can fetch this series from: https://github.com/cota/qemu/tree/tlb-dyn-v2 Note that it applies on top of my tlb-lock-v4 series: https://lists.gnu.org/archive/html/qemu-devel/2018-10/msg01421.html Thanks, Emilio
[Qemu-devel] [RFC v2 3/5] cputlb: do not evict empty entries to the vtlb
Currently we evict an entry to the victim TLB when it doesn't match
the current address. But it could be that there's no match because
the current entry is empty (i.e. all -1's, for instance via tlb_flush).
Do not evict the entry to the vtlb in that case.
This change will help us keep track of the TLB's use rate.
Signed-off-by: Emilio G. Cota
---
include/exec/cpu-all.h | 9 +
accel/tcg/cputlb.c | 2 +-
2 files changed, 10 insertions(+), 1 deletion(-)
diff --git a/include/exec/cpu-all.h b/include/exec/cpu-all.h
index 117d2fbbca..e21140049b 100644
--- a/include/exec/cpu-all.h
+++ b/include/exec/cpu-all.h
@@ -362,6 +362,15 @@ static inline bool tlb_hit(target_ulong tlb_addr,
target_ulong addr)
return tlb_hit_page(tlb_addr, addr & TARGET_PAGE_MASK);
}
+/**
+ * tlb_entry_is_empty - return true if the entry is not in use
+ * @te: pointer to CPUTLBEntry
+ */
+static inline bool tlb_entry_is_empty(const CPUTLBEntry *te)
+{
+return te->addr_read == -1 && te->addr_write == -1 && te->addr_code == -1;
+}
+
void dump_exec_info(FILE *f, fprintf_function cpu_fprintf);
void dump_opcount_info(FILE *f, fprintf_function cpu_fprintf);
#endif /* !CONFIG_USER_ONLY */
diff --git a/accel/tcg/cputlb.c b/accel/tcg/cputlb.c
index 80406f1033..4dc47e603c 100644
--- a/accel/tcg/cputlb.c
+++ b/accel/tcg/cputlb.c
@@ -689,7 +689,7 @@ void tlb_set_page_with_attrs(CPUState *cpu, target_ulong
vaddr,
* Only evict the old entry to the victim tlb if it's for a
* different page; otherwise just overwrite the stale data.
*/
-if (!tlb_hit_page_anyprot(te, vaddr_page)) {
+if (!tlb_hit_page_anyprot(te, vaddr_page) && !tlb_entry_is_empty(te)) {
unsigned vidx = env->vtlb_index++ % CPU_VTLB_SIZE;
CPUTLBEntry *tv = >tlb_v_table[mmu_idx][vidx];
--
2.17.1
[Qemu-devel] [RFC v2 4/5] cputlb: track TLB use rate
This paves the way for implementing a dynamically-sized softmmu.
Signed-off-by: Emilio G. Cota
---
include/exec/cpu-defs.h | 5 +
accel/tcg/cputlb.c | 17 ++---
2 files changed, 19 insertions(+), 3 deletions(-)
diff --git a/include/exec/cpu-defs.h b/include/exec/cpu-defs.h
index 87cd015f60..56f1887c7f 100644
--- a/include/exec/cpu-defs.h
+++ b/include/exec/cpu-defs.h
@@ -141,10 +141,15 @@ typedef struct CPUIOTLBEntry {
MemTxAttrs attrs;
} CPUIOTLBEntry;
+typedef struct CPUTLBDesc {
+size_t n_used_entries;
+} CPUTLBDesc;
+
#define CPU_COMMON_TLB \
/* The meaning of the MMU modes is defined in the target code. */ \
/* tlb_lock serializes updates to tlb_mask, tlb_table and tlb_v_table */ \
QemuSpin tlb_lock; \
+CPUTLBDesc tlb_desc[NB_MMU_MODES]; \
/* tlb_mask[i] contains (n_entries - 1) << CPU_TLB_ENTRY_BITS */\
uintptr_t tlb_mask[NB_MMU_MODES]; \
CPUTLBEntry *tlb_table[NB_MMU_MODES]; \
diff --git a/accel/tcg/cputlb.c b/accel/tcg/cputlb.c
index 4dc47e603c..11d6060eb0 100644
--- a/accel/tcg/cputlb.c
+++ b/accel/tcg/cputlb.c
@@ -82,6 +82,7 @@ void tlb_init(CPUState *cpu)
for (i = 0; i < NB_MMU_MODES; i++) {
size_t n_entries = CPU_TLB_SIZE;
+env->tlb_desc[i].n_used_entries = 0;
env->tlb_mask[i] = (n_entries - 1) << CPU_TLB_ENTRY_BITS;
env->tlb_table[i] = g_new(CPUTLBEntry, n_entries);
env->iotlb[i] = g_new0(CPUIOTLBEntry, n_entries);
@@ -150,6 +151,7 @@ static void tlb_flush_nocheck(CPUState *cpu)
qemu_spin_lock(>tlb_lock);
for (i = 0; i < NB_MMU_MODES; i++) {
memset(env->tlb_table[i], -1, sizeof_tlb(env, i));
+env->tlb_desc[i].n_used_entries = 0;
}
memset(env->tlb_v_table, -1, sizeof(env->tlb_v_table));
qemu_spin_unlock(>tlb_lock);
@@ -213,6 +215,7 @@ static void tlb_flush_by_mmuidx_async_work(CPUState *cpu,
run_on_cpu_data data)
memset(env->tlb_table[mmu_idx], -1, sizeof_tlb(env, mmu_idx));
memset(env->tlb_v_table[mmu_idx], -1, sizeof(env->tlb_v_table[0]));
+env->tlb_desc[mmu_idx].n_used_entries = 0;
}
}
qemu_spin_unlock(>tlb_lock);
@@ -273,12 +276,14 @@ static inline bool tlb_hit_page_anyprot(CPUTLBEntry
*tlb_entry,
}
/* Called with tlb_lock held */
-static inline void tlb_flush_entry_locked(CPUTLBEntry *tlb_entry,
+static inline bool tlb_flush_entry_locked(CPUTLBEntry *tlb_entry,
target_ulong page)
{
if (tlb_hit_page_anyprot(tlb_entry, page)) {
memset(tlb_entry, -1, sizeof(*tlb_entry));
+return true;
}
+return false;
}
/* Called with tlb_lock held */
@@ -316,7 +321,9 @@ static void tlb_flush_page_async_work(CPUState *cpu,
run_on_cpu_data data)
addr &= TARGET_PAGE_MASK;
qemu_spin_lock(>tlb_lock);
for (mmu_idx = 0; mmu_idx < NB_MMU_MODES; mmu_idx++) {
-tlb_flush_entry_locked(tlb_entry(env, mmu_idx, addr), addr);
+if (tlb_flush_entry_locked(tlb_entry(env, mmu_idx, addr), addr)) {
+env->tlb_desc[mmu_idx].n_used_entries--;
+}
tlb_flush_vtlb_page_locked(env, mmu_idx, addr);
}
qemu_spin_unlock(>tlb_lock);
@@ -358,7 +365,9 @@ static void tlb_flush_page_by_mmuidx_async_work(CPUState
*cpu,
qemu_spin_lock(>tlb_lock);
for (mmu_idx = 0; mmu_idx < NB_MMU_MODES; mmu_idx++) {
if (test_bit(mmu_idx, _idx_bitmap)) {
-tlb_flush_entry_locked(tlb_entry(env, mmu_idx, addr), addr);
+if (tlb_flush_entry_locked(tlb_entry(env, mmu_idx, addr), addr)) {
+env->tlb_desc[mmu_idx].n_used_entries--;
+}
tlb_flush_vtlb_page_locked(env, mmu_idx, addr);
}
}
@@ -696,6 +705,7 @@ void tlb_set_page_with_attrs(CPUState *cpu, target_ulong
vaddr,
/* Evict the old entry into the victim tlb. */
copy_tlb_helper_locked(tv, te);
env->iotlb_v[mmu_idx][vidx] = env->iotlb[mmu_idx][index];
+env->tlb_desc[mmu_idx].n_used_entries--;
}
/* refill the tlb */
@@ -747,6 +757,7 @@ void tlb_set_page_with_attrs(CPUState *cpu, target_ulong
vaddr,
}
copy_tlb_helper_locked(te, );
+env->tlb_desc[mmu_idx].n_used_entries++;
qemu_spin_unlock(>tlb_lock);
}
--
2.17.1
[Qemu-devel] [RFC v2 2/5] (XXX) cputlb: introduce indirection for TLB size
This paves the way for implementing dynamic TLB resizing.
XXX: convert other TCG backends
Signed-off-by: Emilio G. Cota
---
include/exec/cpu-defs.h | 10 ++
include/exec/cpu_ldst.h | 14 +-
accel/tcg/cputlb.c| 18 +++---
tcg/i386/tcg-target.inc.c | 28 ++--
4 files changed, 48 insertions(+), 22 deletions(-)
diff --git a/include/exec/cpu-defs.h b/include/exec/cpu-defs.h
index 4ff62f32bf..87cd015f60 100644
--- a/include/exec/cpu-defs.h
+++ b/include/exec/cpu-defs.h
@@ -141,13 +141,15 @@ typedef struct CPUIOTLBEntry {
MemTxAttrs attrs;
} CPUIOTLBEntry;
-#define CPU_COMMON_TLB \
+#define CPU_COMMON_TLB \
/* The meaning of the MMU modes is defined in the target code. */ \
-/* tlb_lock serializes updates to tlb_table and tlb_v_table */ \
+/* tlb_lock serializes updates to tlb_mask, tlb_table and tlb_v_table */ \
QemuSpin tlb_lock; \
-CPUTLBEntry tlb_table[NB_MMU_MODES][CPU_TLB_SIZE]; \
+/* tlb_mask[i] contains (n_entries - 1) << CPU_TLB_ENTRY_BITS */\
+uintptr_t tlb_mask[NB_MMU_MODES]; \
+CPUTLBEntry *tlb_table[NB_MMU_MODES]; \
CPUTLBEntry tlb_v_table[NB_MMU_MODES][CPU_VTLB_SIZE]; \
-CPUIOTLBEntry iotlb[NB_MMU_MODES][CPU_TLB_SIZE];\
+CPUIOTLBEntry *iotlb[NB_MMU_MODES]; \
CPUIOTLBEntry iotlb_v[NB_MMU_MODES][CPU_VTLB_SIZE]; \
size_t tlb_flush_count; \
target_ulong tlb_flush_addr;\
diff --git a/include/exec/cpu_ldst.h b/include/exec/cpu_ldst.h
index e3d8d738aa..3ded1df9b7 100644
--- a/include/exec/cpu_ldst.h
+++ b/include/exec/cpu_ldst.h
@@ -130,7 +130,9 @@ extern __thread uintptr_t helper_retaddr;
static inline uintptr_t tlb_index(CPUArchState *env, uintptr_t mmu_idx,
target_ulong addr)
{
-return (addr >> TARGET_PAGE_BITS) & (CPU_TLB_SIZE - 1);
+uintptr_t size_mask = env->tlb_mask[mmu_idx] >> CPU_TLB_ENTRY_BITS;
+
+return (addr >> TARGET_PAGE_BITS) & size_mask;
}
/* Find the TLB entry corresponding to the mmu_idx + address pair. */
@@ -140,6 +142,16 @@ static inline CPUTLBEntry *tlb_entry(CPUArchState *env,
uintptr_t mmu_idx,
return >tlb_table[mmu_idx][tlb_index(env, mmu_idx, addr)];
}
+static inline size_t sizeof_tlb(CPUArchState *env, uintptr_t mmu_idx)
+{
+return env->tlb_mask[mmu_idx] + (1 << CPU_TLB_ENTRY_BITS);
+}
+
+static inline size_t tlb_n_entries(CPUArchState *env, uintptr_t mmu_idx)
+{
+return (env->tlb_mask[mmu_idx] >> CPU_TLB_ENTRY_BITS) + 1;
+}
+
#ifdef MMU_MODE0_SUFFIX
#define CPU_MMU_INDEX 0
#define MEMSUFFIX MMU_MODE0_SUFFIX
diff --git a/accel/tcg/cputlb.c b/accel/tcg/cputlb.c
index a5972773de..80406f1033 100644
--- a/accel/tcg/cputlb.c
+++ b/accel/tcg/cputlb.c
@@ -76,8 +76,16 @@ QEMU_BUILD_BUG_ON(NB_MMU_MODES > 16);
void tlb_init(CPUState *cpu)
{
CPUArchState *env = cpu->env_ptr;
+int i;
qemu_spin_init(>tlb_lock);
+for (i = 0; i < NB_MMU_MODES; i++) {
+size_t n_entries = CPU_TLB_SIZE;
+
+env->tlb_mask[i] = (n_entries - 1) << CPU_TLB_ENTRY_BITS;
+env->tlb_table[i] = g_new(CPUTLBEntry, n_entries);
+env->iotlb[i] = g_new0(CPUIOTLBEntry, n_entries);
+}
}
/* flush_all_helper: run fn across all cpus
@@ -120,6 +128,7 @@ size_t tlb_flush_count(void)
static void tlb_flush_nocheck(CPUState *cpu)
{
CPUArchState *env = cpu->env_ptr;
+int i;
/* The QOM tests will trigger tlb_flushes without setting up TCG
* so we bug out here in that case.
@@ -139,7 +148,9 @@ static void tlb_flush_nocheck(CPUState *cpu)
* that do not hold the lock are performed by the same owner thread.
*/
qemu_spin_lock(>tlb_lock);
-memset(env->tlb_table, -1, sizeof(env->tlb_table));
+for (i = 0; i < NB_MMU_MODES; i++) {
+memset(env->tlb_table[i], -1, sizeof_tlb(env, i));
+}
memset(env->tlb_v_table, -1, sizeof(env->tlb_v_table));
qemu_spin_unlock(>tlb_lock);
@@ -200,7 +211,7 @@ static void tlb_flush_by_mmuidx_async_work(CPUState *cpu,
run_on_cpu_data data)
if (test_bit(mmu_idx, _idx_bitmask)) {
tlb_debug("%d\n", mmu_idx);
-memset(env->tlb_table[mmu_idx], -1, sizeof(env->tlb_table[0]));
+memset(env->tlb_table[mmu_idx], -1, sizeof_tlb(env, mmu_idx));
memset(env->tlb_v_table[mmu_idx], -1, sizeof(env->tlb_v_table[0]));
}
}
@@ -523,8 +534,9 @@ void tlb_reset_dirty(CPUState *cpu, ram_addr_t start1,
ram_addr_t length)
qemu_spin_lock(>tlb_lock);
for (mmu_idx = 0; mmu_idx < NB_MMU_MODES; mmu_idx++) {
unsigned int i;
+unsigned int n = tlb_n_entries(env, mmu_idx);
-for (i = 0; i <
Re: [Qemu-devel] [PATCH v2 7/7] block/qcow2-refcount: fix out-of-file L2 entries to be read-as-zero
On 10/09/2018 01:21 AM, Max Reitz wrote:
> On 09.10.18 00:14, Vladimir Sementsov-Ogievskiy wrote:
>>
>>
>> On 10/09/2018 01:08 AM, Max Reitz wrote:
>>> On 09.10.18 00:02, Vladimir Sementsov-Ogievskiy wrote:
On 10/08/2018 11:51 PM, Max Reitz wrote:
> On 17.08.18 14:22, Vladimir Sementsov-Ogievskiy wrote:
>> Rewrite corrupted L2 table entry, which reference space out of
>> underlying file.
>>
>> Make this L2 table entry read-as-all-zeros without any allocation.
>>
>> Signed-off-by: Vladimir Sementsov-Ogievskiy
>> ---
>> block/qcow2-refcount.c | 32
>> 1 file changed, 32 insertions(+)
>>
>> diff --git a/block/qcow2-refcount.c b/block/qcow2-refcount.c
>> index 3c004e5bfe..3de3768a3c 100644
>> --- a/block/qcow2-refcount.c
>> +++ b/block/qcow2-refcount.c
>> @@ -1720,8 +1720,30 @@ static int check_refcounts_l2(BlockDriverState
>> *bs, BdrvCheckResult *res,
>> /* Mark cluster as used */
>> csize = (((l2_entry >> s->csize_shift) & s->csize_mask)
>> + 1) *
>> BDRV_SECTOR_SIZE;
>> +if (csize > s->cluster_size) {
>> +ret = fix_l2_entry_to_zero(
>> +bs, res, fix, l2_offset, i, active,
>> +"compressed cluster larger than cluster: size
>> 0x%"
>> +PRIx64, csize);
>> +if (ret < 0) {
>> +goto fail;
>> +}
>> +continue;
>> +}
>> +
>
> This seems recoverable, isn't it? Can we not try to just limit the
> csize, or decompress the cluster with the given csize from the given
> offset, disregarding the cluster limit?
Hm, you want to assume that csize is corrupted but coffset may be
correct? Unlikely, I think.
>>>
>>> Better to reconstruct probably garbage data than to definitely garbage
>>> data (all zeroes) is what I think.
>>>
So, to carefully repair csize, we should decompress one cluster (or one
cluster - 1 byte) of data, trying to get one cluster of decompressed
data. If we succeed, we know csize, or we can safely set it to one cluster.
>>>
>>> Yes.
>>>
Or we can just set csize = 1 cluster, if it is larger. And leave
problems to real execution which will lead to EIO in worst case.
>>>
>>> Or this, yes.
>>>
>> coffset = l2_entry & s->cluster_offset_mask &
>> ~(BDRV_SECTOR_SIZE - 1);
>> +if (coffset >= bdrv_getlength(bs->file->bs)) {
>> +ret = fix_l2_entry_to_zero(
>> +bs, res, fix, l2_offset, i, active,
>> +"compressed cluster out of file: offset 0x%"
>> PRIx64,
>> +coffset);
>> +if (ret < 0) {
>> +goto fail;
>> +}
>> +continue;
>> +}
>> +
>> ret = qcow2_inc_refcounts_imrt(bs, res,
>>refcount_table,
>> refcount_table_size,
>>coffset, csize);
>> @@ -1748,6 +1770,16 @@ static int check_refcounts_l2(BlockDriverState
>> *bs, BdrvCheckResult *res,
>> {
>> uint64_t offset = l2_entry & L2E_OFFSET_MASK;
>>
>> +if (offset >= bdrv_getlength(bs->file->bs)) {
>> +ret = fix_l2_entry_to_zero(
>> +bs, res, fix, l2_offset, i, active,
>> +"cluster out of file: offset 0x%" PRIx64,
>> offset);
>> +if (ret < 0) {
>> +goto fail;
>> +}
>> +continue;
>> +}
>> +
>
> These other two look OK, but they have another issue: If this is a v2
> image, you cannot create zero clusters; so you'll have to unallocate the
> cluster in that case.
Oho, it's a problem. It may be unsafe to discard clusters, making
backing image available through the holes. What discard do on v2?
Zeroing or holes?
>>>
>>> Oh, right! discard on v2 punches a hole. So I see three ways:
>>> (1) You can do the same and point to that bit of code, or
>>> (2) You allocate a data cluster full of zeroes in case of v2, or
>>> (3) You just error out.
>>>
>>> (3) doesn't seem like the worst option.
>>
>>> Amending the image to be v3 is
>>> always possible and trivial.
>>
>> how to do it for corrupted image?
>
> Oh, yeah, you can't open a corrupted image, can you... I suppose we
> want a way to force-clear the flag anyway. :-)
am, which flag?
>
> Max
>
Re: [Qemu-devel] [PATCH v1 11/12] hw/arm: versal: Add a model of Xilinx Versal SoC
On Mon, Oct 08, 2018 at 02:19:09PM +0100, Peter Maydell wrote:
> On 3 October 2018 at 16:07, Edgar E. Iglesias
> wrote:
> > From: "Edgar E. Iglesias"
> >
> > Add a model of Xilinx Versal SoC.
> >
> > Signed-off-by: Edgar E. Iglesias
> > ---
> > default-configs/aarch64-softmmu.mak | 1 +
> > hw/arm/Makefile.objs| 1 +
> > hw/arm/xlnx-versal.c| 339
> >
> > include/hw/arm/xlnx-versal.h| 122 +
> > 4 files changed, 463 insertions(+)
> > create mode 100644 hw/arm/xlnx-versal.c
> > create mode 100644 include/hw/arm/xlnx-versal.h
> >
>
>
> > +#define XLNX_VERSAL_ACPU_TYPE "cortex-a72" "-" TYPE_ARM_CPU
>
> ARM_CPU_TYPE_NAME("cortex-a72") is preferable to hand-assembling
> the type name like this.
Fixed for v2.
>
> > +#define GEM_REVISION0x40070106
> > +
>
> > +for (i = 0; i < nr_apu_cpus; 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;
> > +/* Mapping from the output timer irq lines from the CPU to the
> > + * GIC PPI inputs we use for the virt board.
> > + */
>
> This isn't the virt board :-) -- cut-n-pasted comment ?
Fixed for v2.
>
> > +const int timer_irq[] = {
> > +[GTIMER_PHYS] = VERSAL_TIMER_NS_EL1_IRQ,
> > +[GTIMER_VIRT] = VERSAL_TIMER_VIRT_IRQ,
> > +[GTIMER_HYP] = VERSAL_TIMER_NS_EL2_IRQ,
> > +[GTIMER_SEC] = VERSAL_TIMER_S_EL1_IRQ,
> > +};
> > +
> > +for (ti = 0; ti < ARRAY_SIZE(timer_irq); ti++) {
> > +qdev_connect_gpio_out(cpudev, ti,
> > + qdev_get_gpio_in(gicdev,
> > + ppibase +
> > timer_irq[ti]));
> > +}
> > +maint_irq = qdev_get_gpio_in(gicdev,
> > +ppibase + VERSAL_GIC_MAINT_IRQ);
> > +qdev_connect_gpio_out_named(cpudev, "gicv3-maintenance-interrupt",
> > +0, maint_irq);
> > +sysbus_connect_irq(gicbusdev, i, qdev_get_gpio_in(cpudev,
> > ARM_CPU_IRQ));
> > +sysbus_connect_irq(gicbusdev, i + nr_apu_cpus,
> > + qdev_get_gpio_in(cpudev, ARM_CPU_FIQ));
> > +sysbus_connect_irq(gicbusdev, i + 2 * nr_apu_cpus,
> > + qdev_get_gpio_in(cpudev, ARM_CPU_VIRQ));
> > +sysbus_connect_irq(gicbusdev, i + 3 * nr_apu_cpus,
> > + qdev_get_gpio_in(cpudev, ARM_CPU_VFIQ));
> > +}
> > +
> > +for (i = 0; i < XLNX_VERSAL_NR_IRQS; i++) {
> > +pic[i] = qdev_get_gpio_in(gicdev, i);
> > +}
>
> > +/* This takes the board allocated linear DDR memory and creates aliases
> > + * for each split DDR range/apperture on the Versal address map.
>
> "aperture"
Fixed
>
>
>
> > +static void versal_realize(DeviceState *dev, Error **errp)
> > +{
> > +Versal *s = XLNX_VERSAL(dev);
> > +qemu_irq pic[XLNX_VERSAL_NR_IRQS];
> > +
> > +versal_create_apu_cpus(s, errp);
> > +versal_create_apu_gic(s, pic, errp);
> > +versal_create_uarts(s, pic);
> > +versal_create_gems(s, pic);
> > +versal_map_ddr(s);
> > +versal_unimp(s);
> > +
> > +/* Create the OCM. */
> > +memory_region_init_ram(>lpd.mr_ocm, OBJECT(s), "ocm",
> > + MM_OCM_SIZE, _fatal);
>
> What's an OCM? Is it really memory, or is this a stub for something?
I've changed the comment to spell out that it's an On Chip Memory.
>
> > +
> > +memory_region_add_subregion_overlap(>mr_ps, MM_OCM, >lpd.mr_ocm,
> > 0);
> > +memory_region_add_subregion_overlap(>fpd.apu.mr, 0, >mr_ps, 0);
> > +}
> > +
> > +static void versal_init(Object *obj)
> > +{
> > +Versal *s = XLNX_VERSAL(obj);
> > +
> > +memory_region_init(>fpd.apu.mr, obj, "mr-apu", UINT64_MAX);
> > +memory_region_init(>mr_ps, obj, "mr-ps-switch", UINT64_MAX);
> > +}
> > +
> > +static const VMStateDescription versal_vmstate = {
> > +.name = "xlnx-ve",
> > +.version_id = 1,
> > +.minimum_version_id = 1,
> > +.fields = (VMStateField[]) {
> > +/* FIXME. */
>
> Stray FIXME comment -- I think the answer may be "the
> SoC object has no state of its own so needs neither a
> vmsd nor a reset method" ? (If so and if you drop them,
> do put a comment in the class init about why they're not
> provided. Some day we may have a mechanism for a device
> to explicitly say "I need no vmstate" so we can assert if
> none is provided.)
Thanks, I'll do that.
>
> > +VMSTATE_END_OF_LIST()
> > +}
> > +};
> > +
> > +static Property versal_properties[] = {
> > +DEFINE_PROP_LINK("ddr", Versal, cfg.mr_ddr, TYPE_MEMORY_REGION,
> > + MemoryRegion *),
> > +DEFINE_PROP_UINT32("psci-conduit", Versal, cfg.psci_conduit,
Re: [Qemu-devel] [PATCH v2 7/7] block/qcow2-refcount: fix out-of-file L2 entries to be read-as-zero
On 09.10.18 00:14, Vladimir Sementsov-Ogievskiy wrote:
>
>
> On 10/09/2018 01:08 AM, Max Reitz wrote:
>> On 09.10.18 00:02, Vladimir Sementsov-Ogievskiy wrote:
>>>
>>>
>>> On 10/08/2018 11:51 PM, Max Reitz wrote:
On 17.08.18 14:22, Vladimir Sementsov-Ogievskiy wrote:
> Rewrite corrupted L2 table entry, which reference space out of
> underlying file.
>
> Make this L2 table entry read-as-all-zeros without any allocation.
>
> Signed-off-by: Vladimir Sementsov-Ogievskiy
> ---
>block/qcow2-refcount.c | 32
>1 file changed, 32 insertions(+)
>
> diff --git a/block/qcow2-refcount.c b/block/qcow2-refcount.c
> index 3c004e5bfe..3de3768a3c 100644
> --- a/block/qcow2-refcount.c
> +++ b/block/qcow2-refcount.c
> @@ -1720,8 +1720,30 @@ static int check_refcounts_l2(BlockDriverState
> *bs, BdrvCheckResult *res,
>/* Mark cluster as used */
>csize = (((l2_entry >> s->csize_shift) & s->csize_mask) +
> 1) *
>BDRV_SECTOR_SIZE;
> +if (csize > s->cluster_size) {
> +ret = fix_l2_entry_to_zero(
> +bs, res, fix, l2_offset, i, active,
> +"compressed cluster larger than cluster: size
> 0x%"
> +PRIx64, csize);
> +if (ret < 0) {
> +goto fail;
> +}
> +continue;
> +}
> +
This seems recoverable, isn't it? Can we not try to just limit the
csize, or decompress the cluster with the given csize from the given
offset, disregarding the cluster limit?
>>>
>>> Hm, you want to assume that csize is corrupted but coffset may be
>>> correct? Unlikely, I think.
>>
>> Better to reconstruct probably garbage data than to definitely garbage
>> data (all zeroes) is what I think.
>>
>>> So, to carefully repair csize, we should decompress one cluster (or one
>>> cluster - 1 byte) of data, trying to get one cluster of decompressed
>>> data. If we succeed, we know csize, or we can safely set it to one cluster.
>>
>> Yes.
>>
>>> Or we can just set csize = 1 cluster, if it is larger. And leave
>>> problems to real execution which will lead to EIO in worst case.
>>
>> Or this, yes.
>>
>coffset = l2_entry & s->cluster_offset_mask &
> ~(BDRV_SECTOR_SIZE - 1);
> +if (coffset >= bdrv_getlength(bs->file->bs)) {
> +ret = fix_l2_entry_to_zero(
> +bs, res, fix, l2_offset, i, active,
> +"compressed cluster out of file: offset 0x%"
> PRIx64,
> +coffset);
> +if (ret < 0) {
> +goto fail;
> +}
> +continue;
> +}
> +
>ret = qcow2_inc_refcounts_imrt(bs, res,
> refcount_table,
> refcount_table_size,
> coffset, csize);
> @@ -1748,6 +1770,16 @@ static int check_refcounts_l2(BlockDriverState
> *bs, BdrvCheckResult *res,
>{
>uint64_t offset = l2_entry & L2E_OFFSET_MASK;
>
> +if (offset >= bdrv_getlength(bs->file->bs)) {
> +ret = fix_l2_entry_to_zero(
> +bs, res, fix, l2_offset, i, active,
> +"cluster out of file: offset 0x%" PRIx64,
> offset);
> +if (ret < 0) {
> +goto fail;
> +}
> +continue;
> +}
> +
These other two look OK, but they have another issue: If this is a v2
image, you cannot create zero clusters; so you'll have to unallocate the
cluster in that case.
>>>
>>>
>>> Oho, it's a problem. It may be unsafe to discard clusters, making
>>> backing image available through the holes. What discard do on v2?
>>> Zeroing or holes?
>>
>> Oh, right! discard on v2 punches a hole. So I see three ways:
>> (1) You can do the same and point to that bit of code, or
>> (2) You allocate a data cluster full of zeroes in case of v2, or
>> (3) You just error out.
>>
>> (3) doesn't seem like the worst option.
>
>> Amending the image to be v3 is
>> always possible and trivial.
>
> how to do it for corrupted image?
Oh, yeah, you can't open a corrupted image, can you... I suppose we
want a way to force-clear the flag anyway. :-)
Max
signature.asc
Description: OpenPGP digital signature
[Qemu-devel] possible BUG, ATS12NSOPW fails
Hi Peter, I am chasing an address translation error, and it looks like it might be a QEMU bug, because I cannot reproduce the problem on a physical board. The issue is that a requested ATS12NSOPW translation in Xen is reported as failing by QEMU, but actually the address is correct. The workflow is as follows: 1) Linux as Dom0 registers a memory area with Xen at boot, see: drivers/xen/time.c:xen_setup_runstate_info in Linux 2) Xen tries to update such region with new data every so often, see: xen/arch/arm/domain.c:update_runstate_area As you can see from the code, Xen uses __raw_copy_to_guest and __copy_to_guest to copy the new values to the guest. Both of them eventually call xen/arch/arm/guestcopy.c:copy_guest: copy_guest -> translate_get_page -> get_page_from_gva -> gvirt_to_maddr to do the translation because Xen only memorizes the guest virtual address as passed by xen_setup_runstate_info in Linux. gvirt_to_maddr uses ATS12NSOPW for the translation. I double-checked all the addresses and they are correct. For some reason after starting the guest, ATS12NSOPW starts to fail for ffc006f91628, which is the same virtual address corresponding to the same runstate region allocated in Linux. I added a couple of printf's to QEMU and Xen: (XEN) DEBUG do_vcpu_op 1458 area.v=ffc006f91628 ### This is the virtual address as passed by Linux [...] (QEMU) DEBUG do_ats_write 2256 ret=1 phys_addr=200 value=ffc006f91628 (QEMU) DEBUG do_ats_write 2326 par64=80b ### This is the output from QEMU, I added the printks to do_ats_write, line numbers 2256 and 2326 (XEN) p2m.c:1426: d1v0: gvirt_to_maddr failed va=0xffc006f91628 flags=0x1 par=0x80b (XEN) DEBUG translate_get_page 42 addr=ffc006f91628 linear=1 write=1 (XEN) DEBUG raw_copy_to_guest 120 caller=domain.c#update_runstate_area+0x78/0x10c (XEN) DEBUG update_runstate_area 294 ### This is Xen failing the copy_to_guest in update_runstate_area. Do you have any ideas on what's wrong? I am happy to run more tests with QEMU to get more data. Cheers, Stefano
Re: [Qemu-devel] [PATCH v2 7/7] block/qcow2-refcount: fix out-of-file L2 entries to be read-as-zero
On 10/09/2018 01:08 AM, Max Reitz wrote:
> On 09.10.18 00:02, Vladimir Sementsov-Ogievskiy wrote:
>>
>>
>> On 10/08/2018 11:51 PM, Max Reitz wrote:
>>> On 17.08.18 14:22, Vladimir Sementsov-Ogievskiy wrote:
Rewrite corrupted L2 table entry, which reference space out of
underlying file.
Make this L2 table entry read-as-all-zeros without any allocation.
Signed-off-by: Vladimir Sementsov-Ogievskiy
---
block/qcow2-refcount.c | 32
1 file changed, 32 insertions(+)
diff --git a/block/qcow2-refcount.c b/block/qcow2-refcount.c
index 3c004e5bfe..3de3768a3c 100644
--- a/block/qcow2-refcount.c
+++ b/block/qcow2-refcount.c
@@ -1720,8 +1720,30 @@ static int check_refcounts_l2(BlockDriverState *bs,
BdrvCheckResult *res,
/* Mark cluster as used */
csize = (((l2_entry >> s->csize_shift) & s->csize_mask) +
1) *
BDRV_SECTOR_SIZE;
+if (csize > s->cluster_size) {
+ret = fix_l2_entry_to_zero(
+bs, res, fix, l2_offset, i, active,
+"compressed cluster larger than cluster: size 0x%"
+PRIx64, csize);
+if (ret < 0) {
+goto fail;
+}
+continue;
+}
+
>>>
>>> This seems recoverable, isn't it? Can we not try to just limit the
>>> csize, or decompress the cluster with the given csize from the given
>>> offset, disregarding the cluster limit?
>>
>> Hm, you want to assume that csize is corrupted but coffset may be
>> correct? Unlikely, I think.
>
> Better to reconstruct probably garbage data than to definitely garbage
> data (all zeroes) is what I think.
>
>> So, to carefully repair csize, we should decompress one cluster (or one
>> cluster - 1 byte) of data, trying to get one cluster of decompressed
>> data. If we succeed, we know csize, or we can safely set it to one cluster.
>
> Yes.
>
>> Or we can just set csize = 1 cluster, if it is larger. And leave
>> problems to real execution which will lead to EIO in worst case.
>
> Or this, yes.
>
coffset = l2_entry & s->cluster_offset_mask &
~(BDRV_SECTOR_SIZE - 1);
+if (coffset >= bdrv_getlength(bs->file->bs)) {
+ret = fix_l2_entry_to_zero(
+bs, res, fix, l2_offset, i, active,
+"compressed cluster out of file: offset 0x%"
PRIx64,
+coffset);
+if (ret < 0) {
+goto fail;
+}
+continue;
+}
+
ret = qcow2_inc_refcounts_imrt(bs, res,
refcount_table,
refcount_table_size,
coffset, csize);
@@ -1748,6 +1770,16 @@ static int check_refcounts_l2(BlockDriverState *bs,
BdrvCheckResult *res,
{
uint64_t offset = l2_entry & L2E_OFFSET_MASK;
+if (offset >= bdrv_getlength(bs->file->bs)) {
+ret = fix_l2_entry_to_zero(
+bs, res, fix, l2_offset, i, active,
+"cluster out of file: offset 0x%" PRIx64, offset);
+if (ret < 0) {
+goto fail;
+}
+continue;
+}
+
>>>
>>> These other two look OK, but they have another issue: If this is a v2
>>> image, you cannot create zero clusters; so you'll have to unallocate the
>>> cluster in that case.
>>
>>
>> Oho, it's a problem. It may be unsafe to discard clusters, making
>> backing image available through the holes. What discard do on v2?
>> Zeroing or holes?
>
> Oh, right! discard on v2 punches a hole. So I see three ways:
> (1) You can do the same and point to that bit of code, or
> (2) You allocate a data cluster full of zeroes in case of v2, or
> (3) You just error out.
>
> (3) doesn't seem like the worst option.
> Amending the image to be v3 is
> always possible and trivial.
how to do it for corrupted image?
> Maybe point the user to that option.
>
> Max
>
Re: [Qemu-devel] [PATCH v2 7/7] block/qcow2-refcount: fix out-of-file L2 entries to be read-as-zero
On 09.10.18 00:02, Vladimir Sementsov-Ogievskiy wrote:
>
>
> On 10/08/2018 11:51 PM, Max Reitz wrote:
>> On 17.08.18 14:22, Vladimir Sementsov-Ogievskiy wrote:
>>> Rewrite corrupted L2 table entry, which reference space out of
>>> underlying file.
>>>
>>> Make this L2 table entry read-as-all-zeros without any allocation.
>>>
>>> Signed-off-by: Vladimir Sementsov-Ogievskiy
>>> ---
>>> block/qcow2-refcount.c | 32
>>> 1 file changed, 32 insertions(+)
>>>
>>> diff --git a/block/qcow2-refcount.c b/block/qcow2-refcount.c
>>> index 3c004e5bfe..3de3768a3c 100644
>>> --- a/block/qcow2-refcount.c
>>> +++ b/block/qcow2-refcount.c
>>> @@ -1720,8 +1720,30 @@ static int check_refcounts_l2(BlockDriverState *bs,
>>> BdrvCheckResult *res,
>>> /* Mark cluster as used */
>>> csize = (((l2_entry >> s->csize_shift) & s->csize_mask) + 1) *
>>> BDRV_SECTOR_SIZE;
>>> +if (csize > s->cluster_size) {
>>> +ret = fix_l2_entry_to_zero(
>>> +bs, res, fix, l2_offset, i, active,
>>> +"compressed cluster larger than cluster: size 0x%"
>>> +PRIx64, csize);
>>> +if (ret < 0) {
>>> +goto fail;
>>> +}
>>> +continue;
>>> +}
>>> +
>>
>> This seems recoverable, isn't it? Can we not try to just limit the
>> csize, or decompress the cluster with the given csize from the given
>> offset, disregarding the cluster limit?
>
> Hm, you want to assume that csize is corrupted but coffset may be
> correct? Unlikely, I think.
Better to reconstruct probably garbage data than to definitely garbage
data (all zeroes) is what I think.
> So, to carefully repair csize, we should decompress one cluster (or one
> cluster - 1 byte) of data, trying to get one cluster of decompressed
> data. If we succeed, we know csize, or we can safely set it to one cluster.
Yes.
> Or we can just set csize = 1 cluster, if it is larger. And leave
> problems to real execution which will lead to EIO in worst case.
Or this, yes.
>>> coffset = l2_entry & s->cluster_offset_mask &
>>> ~(BDRV_SECTOR_SIZE - 1);
>>> +if (coffset >= bdrv_getlength(bs->file->bs)) {
>>> +ret = fix_l2_entry_to_zero(
>>> +bs, res, fix, l2_offset, i, active,
>>> +"compressed cluster out of file: offset 0x%"
>>> PRIx64,
>>> +coffset);
>>> +if (ret < 0) {
>>> +goto fail;
>>> +}
>>> +continue;
>>> +}
>>> +
>>> ret = qcow2_inc_refcounts_imrt(bs, res,
>>> refcount_table,
>>> refcount_table_size,
>>> coffset, csize);
>>> @@ -1748,6 +1770,16 @@ static int check_refcounts_l2(BlockDriverState *bs,
>>> BdrvCheckResult *res,
>>> {
>>> uint64_t offset = l2_entry & L2E_OFFSET_MASK;
>>>
>>> +if (offset >= bdrv_getlength(bs->file->bs)) {
>>> +ret = fix_l2_entry_to_zero(
>>> +bs, res, fix, l2_offset, i, active,
>>> +"cluster out of file: offset 0x%" PRIx64, offset);
>>> +if (ret < 0) {
>>> +goto fail;
>>> +}
>>> +continue;
>>> +}
>>> +
>>
>> These other two look OK, but they have another issue: If this is a v2
>> image, you cannot create zero clusters; so you'll have to unallocate the
>> cluster in that case.
>
>
> Oho, it's a problem. It may be unsafe to discard clusters, making
> backing image available through the holes. What discard do on v2?
> Zeroing or holes?
Oh, right! discard on v2 punches a hole. So I see three ways:
(1) You can do the same and point to that bit of code, or
(2) You allocate a data cluster full of zeroes in case of v2, or
(3) You just error out.
(3) doesn't seem like the worst option. Amending the image to be v3 is
always possible and trivial. Maybe point the user to that option.
Max
signature.asc
Description: OpenPGP digital signature
Re: [Qemu-devel] [PATCH v2 04/12] qemu-option: improve qemu_opts_print_help() output
First of all, this patch broke iotest 082. But then again, all that'd
be needed is a correction of the reference output.
However:
On 07.09.18 09:59, Marc-André Lureau wrote:
> Modify qemu_opts_print_help():
> - to print expected argument type
> - skip description if not available
> - sort lines
> - prefix with the list name (like qdev, to avoid confusion)
Is this really useful? My main issue right now is this:
$ qemu-img amend -f qcow2 -o help
Creation options for 'qcow2':
qcow2-create-opts.backing_file=str - File name of a base image
qcow2-create-opts.backing_fmt=str - Image format of the base image
qcow2-create-opts.cluster_size=size - qcow2 cluster size
[...]
(The reason create is not affected is because it copies the options into
a new list.)
This is supposed to be a human-readable output. I don't see how the
rather internal list name[1] really helps here.
([1] I suppose if you write a configuration file, these identifiers
become visible. But you don't use "help" in a configuration file, so I
find that point becomes rather moot.)
So this is why I don't think it is a good idea to print this name.
Now if it helped to prevent confusion, OK, but without further
explanation I don't see how. Is this because I can use "help" in places
where it's unclear to the user what exactly the "help" refers to?
Also, this is bikeshedding, but still, I don't like the dot notation
here. It doesn't mean anything (I can't use
"qcow2-create-opts.backing_file", nor can I use
"virtio-blk-pci.iothread"), so I'd prefer a nice "human" prefix like
"(qcow2-create-opts) backing_file" or "qcow2-create-opts: ".
Actually, I don't like the whole notation. It looks like code. Here is
an excerpt from -device virtio-blk-pci,help:
virtio-blk-pci.iothread=link
virtio-blk-pci.request-merging=bool (on/off)
virtio-blk-pci.logical_block_size=uint16 (A power of two between 512 and
32768)
I can imagine some ways this would be more readable to human users (and
I don't think ease of parsing is a high priority here), for instance:
virtio-blk-pci options:
iothread: link to object of type iothread
request-merging: bool (on/off)
logical_block_size: uint16 (A power of two between 512 and 32768)
(But it appears that transforming "link" to something readable
wouldn't be easy. Though then again, I really think it's unreadable
unless you know what it's supposed to mean.)
So my concrete requests are this:
1. If the ID is really useful, I would put it above the whole list in an
own line. It's not useful to human readers to re-print it on every
single line. It would be nice to have an option to disable this line,
however, if the caller has already printed something along the lines
(which qemu-img amend does).
2. Put some more whitespace into the whole thing, and make it less
robot-y overall. I much prefer ": " when reading over "=" (even
programming languages do when it's about types, in fact).
I'm not writing a patch yet because maybe there is some reason to print
the ID on every single line. So I'm just proposing first.
Max
> - drop 16-chars alignment, use a '-' as seperator for option name and
> description
>
> For ex, "-spice help" output is changed from:
>
> port No description available
> tls-port No description available
> addr No description available
> [...]
> gl No description available
> rendernode No description available
>
> to:
>
> spice.addr=str
> spice.agent-mouse=bool (on/off)
> spice.disable-agent-file-xfer=bool (on/off)
> [...]
> spice.x509-key-password=str
> spice.zlib-glz-wan-compression=str
>
> "qemu-img create -f qcow2 -o help", changed from:
>
> size Virtual disk size
> compat Compatibility level (0.10 or 1.1)
> backing_file File name of a base image
> [...]
> lazy_refcounts Postpone refcount updates
> refcount_bitsWidth of a reference count entry in bits
>
> to:
>
> backing_file=str - File name of a base image
> backing_fmt=str - Image format of the base image
> cluster_size=size - qcow2 cluster size
> [...]
> refcount_bits=num - Width of a reference count entry in bits
> size=size - Virtual disk size
>
> Signed-off-by: Marc-André Lureau
> Reviewed-by: Eric Blake
> ---
> util/qemu-option.c | 38 --
> 1 file changed, 36 insertions(+), 2 deletions(-)
>
> diff --git a/util/qemu-option.c b/util/qemu-option.c
> index 557b6c6626..069de36d2c 100644
> --- a/util/qemu-option.c
> +++ b/util/qemu-option.c
> @@ -208,17 +208,51 @@ out:
> return result;
> }
>
> +static const char *opt_type_to_string(enum QemuOptType type)
> +{
> +switch (type) {
> +case QEMU_OPT_STRING:
> +return "str";
> +case QEMU_OPT_BOOL:
> +return "bool (on/off)";
> +case QEMU_OPT_NUMBER:
> +return "num";
> +case QEMU_OPT_SIZE:
> +return "size";
> +}
> +
> +g_assert_not_reached();
> +}
> +
> void qemu_opts_print_help(QemuOptsList *list)
> {
Re: [Qemu-devel] [PATCH v2 7/7] block/qcow2-refcount: fix out-of-file L2 entries to be read-as-zero
On 10/08/2018 11:51 PM, Max Reitz wrote:
> On 17.08.18 14:22, Vladimir Sementsov-Ogievskiy wrote:
>> Rewrite corrupted L2 table entry, which reference space out of
>> underlying file.
>>
>> Make this L2 table entry read-as-all-zeros without any allocation.
>>
>> Signed-off-by: Vladimir Sementsov-Ogievskiy
>> ---
>> block/qcow2-refcount.c | 32
>> 1 file changed, 32 insertions(+)
>>
>> diff --git a/block/qcow2-refcount.c b/block/qcow2-refcount.c
>> index 3c004e5bfe..3de3768a3c 100644
>> --- a/block/qcow2-refcount.c
>> +++ b/block/qcow2-refcount.c
>> @@ -1720,8 +1720,30 @@ static int check_refcounts_l2(BlockDriverState *bs,
>> BdrvCheckResult *res,
>> /* Mark cluster as used */
>> csize = (((l2_entry >> s->csize_shift) & s->csize_mask) + 1) *
>> BDRV_SECTOR_SIZE;
>> +if (csize > s->cluster_size) {
>> +ret = fix_l2_entry_to_zero(
>> +bs, res, fix, l2_offset, i, active,
>> +"compressed cluster larger than cluster: size 0x%"
>> +PRIx64, csize);
>> +if (ret < 0) {
>> +goto fail;
>> +}
>> +continue;
>> +}
>> +
>
> This seems recoverable, isn't it? Can we not try to just limit the
> csize, or decompress the cluster with the given csize from the given
> offset, disregarding the cluster limit?
Hm, you want to assume that csize is corrupted but coffset may be
correct? Unlikely, I think.
So, to carefully repair csize, we should decompress one cluster (or one
cluster - 1 byte) of data, trying to get one cluster of decompressed
data. If we succeed, we know csize, or we can safely set it to one cluster.
Or we can just set csize = 1 cluster, if it is larger. And leave
problems to real execution which will lead to EIO in worst case.
>
>> coffset = l2_entry & s->cluster_offset_mask &
>> ~(BDRV_SECTOR_SIZE - 1);
>> +if (coffset >= bdrv_getlength(bs->file->bs)) {
>> +ret = fix_l2_entry_to_zero(
>> +bs, res, fix, l2_offset, i, active,
>> +"compressed cluster out of file: offset 0x%" PRIx64,
>> +coffset);
>> +if (ret < 0) {
>> +goto fail;
>> +}
>> +continue;
>> +}
>> +
>> ret = qcow2_inc_refcounts_imrt(bs, res,
>> refcount_table,
>> refcount_table_size,
>> coffset, csize);
>> @@ -1748,6 +1770,16 @@ static int check_refcounts_l2(BlockDriverState *bs,
>> BdrvCheckResult *res,
>> {
>> uint64_t offset = l2_entry & L2E_OFFSET_MASK;
>>
>> +if (offset >= bdrv_getlength(bs->file->bs)) {
>> +ret = fix_l2_entry_to_zero(
>> +bs, res, fix, l2_offset, i, active,
>> +"cluster out of file: offset 0x%" PRIx64, offset);
>> +if (ret < 0) {
>> +goto fail;
>> +}
>> +continue;
>> +}
>> +
>
> These other two look OK, but they have another issue: If this is a v2
> image, you cannot create zero clusters; so you'll have to unallocate the
> cluster in that case.
Oho, it's a problem. It may be unsafe to discard clusters, making
backing image available through the holes. What discard do on v2?
Zeroing or holes?
>
> Max
>
[Qemu-devel] [Bug 1796754] Re: ioctl SIOCGIFCONF causes qemu-aarch64-static to crash with "received signal outside vCPU context"
I was hit by this issue when I tried to run some Java program. And it turns out jdk sets the buf to NULL: http://hg.openjdk.java.net/jdk7/jdk7/jdk/file/887e525597f8/src/solaris/native/java/net/NetworkInterface.c#l1042 Setting to NULL is valid according to http://man7.org/linux/man- pages/man7/netdevice.7.html But qemu doesn’t handle the case: https://github.com/qemu/qemu/blob/aa8e26de9617756febcbf794dda965df307fdaaa /linux-user/syscall.c#L4105 I guess qemu developers didn’t handle the case because the Linux kernel changed and they were based on behavior of old version: https://linux.die.net/man/7/netdevice Please add the support for it otherwise a wide range of network related Java programs won’t run. -- You received this bug notification because you are a member of qemu- devel-ml, which is subscribed to QEMU. https://bugs.launchpad.net/bugs/1796754 Title: ioctl SIOCGIFCONF causes qemu-aarch64-static to crash with "received signal outside vCPU context" Status in QEMU: New Bug description: To reproduce it, compile the attached crash.c under aarch64 to a.out and execute on x86_64 qemu-aarch64-static ./a.out It will print the following and crash: socket=3 qemu:handle_cpu_signal received signal outside vCPU context @ pc=0x60038cd6 qemu:handle_cpu_signal received signal outside vCPU context @ pc=0x6000157a The version of qemu-aarch64-static is qemu-aarch64 version 3.0.0 (qemu-3.0.0-1.fc29) Copyright (c) 2003-2017 Fabrice Bellard and the QEMU Project developers But it did also happen in previous versions so it is not a regression but a bug existed ever since. To manage notifications about this bug go to: https://bugs.launchpad.net/qemu/+bug/1796754/+subscriptions
Re: [Qemu-devel] [PATCH] qemu-system-hppa: Raise exception 26 on emulated hardware
On 10/7/18 1:51 PM, Helge Deller wrote: > On PCXS chips (PA7000, pa 1.1a), trap #18 is raised on memory faults, > while all later chips (>= PA7100) generate either trap #26, #27 or #28 > (depending on the fault type). > > Since the current qemu emulation emulates a B160L machine (with a > PA7300LC PCX-L2 chip, we should raise trap #26 (EXCP_DMAR) instead of > #18 (EXCP_DMP) on access faults by the Linux kernel to page zero. > > With the patch we now get the correct output (I tested against real > hardware): > Kernel Fault: Code=26 (Data memory access rights trap) (Addr=0004) > instead of: > Kernel Fault: Code=18 (Data memory protection/unaligned access trap) > (Addr=0004) > > Signed-off-by: Helge Deller Queued, thanks. r~
[Qemu-devel] [PATCH v3 03/10] target/arm: Convert v8 extensions from feature bits to isar tests
Most of the v8 extensions are self-contained within the ISAR
registers and are not implied by other feature bits, which
makes them the easiest to convert.
Reviewed-by: Philippe Mathieu-Daudé
Signed-off-by: Richard Henderson
---
target/arm/cpu.h | 123 +
target/arm/translate-a64.h | 20 ++
target/arm/translate.h | 16 +
linux-user/elfload.c | 46 --
target/arm/cpu.c | 27 +---
target/arm/cpu64.c | 52 ++--
target/arm/translate-a64.c | 101 +++---
target/arm/translate.c | 36 +--
8 files changed, 294 insertions(+), 127 deletions(-)
diff --git a/target/arm/cpu.h b/target/arm/cpu.h
index f00c0444c4..66eaa40ed9 100644
--- a/target/arm/cpu.h
+++ b/target/arm/cpu.h
@@ -1573,30 +1573,18 @@ enum arm_features {
ARM_FEATURE_LPAE, /* has Large Physical Address Extension */
ARM_FEATURE_V8,
ARM_FEATURE_AARCH64, /* supports 64 bit mode */
-ARM_FEATURE_V8_AES, /* implements AES part of v8 Crypto Extensions */
ARM_FEATURE_CBAR, /* has cp15 CBAR */
ARM_FEATURE_CRC, /* ARMv8 CRC instructions */
ARM_FEATURE_CBAR_RO, /* has cp15 CBAR and it is read-only */
ARM_FEATURE_EL2, /* has EL2 Virtualization support */
ARM_FEATURE_EL3, /* has EL3 Secure monitor support */
-ARM_FEATURE_V8_SHA1, /* implements SHA1 part of v8 Crypto Extensions */
-ARM_FEATURE_V8_SHA256, /* implements SHA256 part of v8 Crypto Extensions */
-ARM_FEATURE_V8_PMULL, /* implements PMULL part of v8 Crypto Extensions */
ARM_FEATURE_THUMB_DSP, /* DSP insns supported in the Thumb encodings */
ARM_FEATURE_PMU, /* has PMU support */
ARM_FEATURE_VBAR, /* has cp15 VBAR */
ARM_FEATURE_M_SECURITY, /* M profile Security Extension */
ARM_FEATURE_JAZELLE, /* has (trivial) Jazelle implementation */
ARM_FEATURE_SVE, /* has Scalable Vector Extension */
-ARM_FEATURE_V8_SHA512, /* implements SHA512 part of v8 Crypto Extensions */
-ARM_FEATURE_V8_SHA3, /* implements SHA3 part of v8 Crypto Extensions */
-ARM_FEATURE_V8_SM3, /* implements SM3 part of v8 Crypto Extensions */
-ARM_FEATURE_V8_SM4, /* implements SM4 part of v8 Crypto Extensions */
-ARM_FEATURE_V8_ATOMICS, /* ARMv8.1-Atomics feature */
-ARM_FEATURE_V8_RDM, /* implements v8.1 simd round multiply */
-ARM_FEATURE_V8_DOTPROD, /* implements v8.2 simd dot product */
ARM_FEATURE_V8_FP16, /* implements v8.2 half-precision float */
-ARM_FEATURE_V8_FCMA, /* has complex number part of v8.3 extensions. */
ARM_FEATURE_M_MAIN, /* M profile Main Extension */
};
@@ -3148,4 +3136,115 @@ static inline uint64_t *aa64_vfp_qreg(CPUARMState *env,
unsigned regno)
/* Shared between translate-sve.c and sve_helper.c. */
extern const uint64_t pred_esz_masks[4];
+/*
+ * 32-bit feature tests via id registers.
+ */
+static inline bool aa32_feature_aes(ARMCPU *cpu)
+{
+return FIELD_EX32(cpu->id_isar5, ID_ISAR5, AES) != 0;
+}
+
+static inline bool aa32_feature_pmull(ARMCPU *cpu)
+{
+return FIELD_EX32(cpu->id_isar5, ID_ISAR5, AES) > 1;
+}
+
+static inline bool aa32_feature_sha1(ARMCPU *cpu)
+{
+return FIELD_EX32(cpu->id_isar5, ID_ISAR5, SHA1) != 0;
+}
+
+static inline bool aa32_feature_sha2(ARMCPU *cpu)
+{
+return FIELD_EX32(cpu->id_isar5, ID_ISAR5, SHA2) != 0;
+}
+
+static inline bool aa32_feature_crc32(ARMCPU *cpu)
+{
+return FIELD_EX32(cpu->id_isar5, ID_ISAR5, CRC32) != 0;
+}
+
+static inline bool aa32_feature_rdm(ARMCPU *cpu)
+{
+return FIELD_EX32(cpu->id_isar5, ID_ISAR5, RDM) != 0;
+}
+
+static inline bool aa32_feature_vcma(ARMCPU *cpu)
+{
+return FIELD_EX32(cpu->id_isar5, ID_ISAR5, VCMA) != 0;
+}
+
+static inline bool aa32_feature_dp(ARMCPU *cpu)
+{
+return FIELD_EX32(cpu->id_isar6, ID_ISAR6, DP) != 0;
+}
+
+/*
+ * 64-bit feature tests via id registers.
+ */
+static inline bool aa64_feature_aes(ARMCPU *cpu)
+{
+return FIELD_EX64(cpu->id_aa64isar0, ID_AA64ISAR0, AES) != 0;
+}
+
+static inline bool aa64_feature_pmull(ARMCPU *cpu)
+{
+return FIELD_EX64(cpu->id_aa64isar0, ID_AA64ISAR0, AES) > 1;
+}
+
+static inline bool aa64_feature_sha1(ARMCPU *cpu)
+{
+return FIELD_EX64(cpu->id_aa64isar0, ID_AA64ISAR0, SHA1) != 0;
+}
+
+static inline bool aa64_feature_sha256(ARMCPU *cpu)
+{
+return FIELD_EX64(cpu->id_aa64isar0, ID_AA64ISAR0, SHA2) != 0;
+}
+
+static inline bool aa64_feature_sha512(ARMCPU *cpu)
+{
+return FIELD_EX64(cpu->id_aa64isar0, ID_AA64ISAR0, SHA2) > 1;
+}
+
+static inline bool aa64_feature_crc32(ARMCPU *cpu)
+{
+return FIELD_EX64(cpu->id_aa64isar0, ID_AA64ISAR0, CRC32) != 0;
+}
+
+static inline bool aa64_feature_atomics(ARMCPU *cpu)
+{
+return FIELD_EX64(cpu->id_aa64isar0, ID_AA64ISAR0, ATOMIC) != 0;
+}
+
+static inline bool aa64_feature_rdm(ARMCPU *cpu)
+{
+return FIELD_EX64(cpu->id_aa64isar0, ID_AA64ISAR0, RDM) != 0;
+}
+
+static inline bool aa64_feature_sha3(ARMCPU *cpu)
+{
+return
[Qemu-devel] [PATCH v3 00/10] target/arm: Rely on id regs instead of features
This edition fixes a number of conflicts with master, and adds a few field definitions from ARMv8.5, courtesy of Philippe. It also fixes a big think-o in a last-minute change to the sve system mode patch set that was applied to master today. That would be patch 1. Sorry for not testing the original more thoroughly. r~ Richard Henderson (10): target/arm: Fix aarch64_sve_change_el wrt EL0 target/arm: Define fields of ISAR registers target/arm: Convert v8 extensions from feature bits to isar tests target/arm: Align cortex-r5 id_isar0 target/arm: Fix cortex-a7 id_isar0 target/arm: Convert division from feature bits to isar0 tests target/arm: Convert jazelle from feature bit to isar1 test target/arm: Convert t32ee from feature bit to isar3 test target/arm: Convert sve from feature bit to aa64pfr0 test target/arm: Convert v8.2-fp16 from feature bit to aa64pfr0 test target/arm/cpu.h| 275 +--- target/arm/translate-a64.h | 22 +++ target/arm/translate.h | 20 +++ linux-user/aarch64/signal.c | 4 +- linux-user/elfload.c| 60 linux-user/syscall.c| 10 +- target/arm/cpu.c| 65 + target/arm/cpu64.c | 66 + target/arm/helper.c | 29 ++-- target/arm/machine.c| 6 +- target/arm/op_helper.c | 6 +- target/arm/translate-a64.c | 145 ++- target/arm/translate.c | 48 +++ 13 files changed, 538 insertions(+), 218 deletions(-) -- 2.17.1
[Qemu-devel] [PATCH v3 01/10] target/arm: Fix aarch64_sve_change_el wrt EL0
At present we assert:
arm_el_is_aa64: Assertion `el >= 1 && el <= 3' failed.
The comment in arm_el_is_aa64 explains why asking about EL0 without
extra information is impossible. Add an extra argument to provide
it from the surrounding context.
Fixes: 0ab5953b00b3
Signed-off-by: Richard Henderson
---
target/arm/cpu.h | 7 +--
target/arm/helper.c| 16
target/arm/op_helper.c | 6 +-
3 files changed, 22 insertions(+), 7 deletions(-)
diff --git a/target/arm/cpu.h b/target/arm/cpu.h
index 3a2aff1192..54362ddce8 100644
--- a/target/arm/cpu.h
+++ b/target/arm/cpu.h
@@ -911,10 +911,13 @@ int arm_cpu_write_elf32_note(WriteCoreDumpFunction f,
CPUState *cs,
int aarch64_cpu_gdb_read_register(CPUState *cpu, uint8_t *buf, int reg);
int aarch64_cpu_gdb_write_register(CPUState *cpu, uint8_t *buf, int reg);
void aarch64_sve_narrow_vq(CPUARMState *env, unsigned vq);
-void aarch64_sve_change_el(CPUARMState *env, int old_el, int new_el);
+void aarch64_sve_change_el(CPUARMState *env, int old_el,
+ int new_el, bool el0_a64);
#else
static inline void aarch64_sve_narrow_vq(CPUARMState *env, unsigned vq) { }
-static inline void aarch64_sve_change_el(CPUARMState *env, int o, int n) { }
+static inline void aarch64_sve_change_el(CPUARMState *env, int o,
+ int n, bool a)
+{ }
#endif
target_ulong do_arm_semihosting(CPUARMState *env);
diff --git a/target/arm/helper.c b/target/arm/helper.c
index c83f7c1109..0efbb5c76c 100644
--- a/target/arm/helper.c
+++ b/target/arm/helper.c
@@ -8374,7 +8374,11 @@ static void arm_cpu_do_interrupt_aarch64(CPUState *cs)
unsigned int new_mode = aarch64_pstate_mode(new_el, true);
unsigned int cur_el = arm_current_el(env);
-aarch64_sve_change_el(env, cur_el, new_el);
+/*
+ * Note that new_el can never be 0. If cur_el is 0, then
+ * el0_a64 is is_a64(), else el0_a64 is ignored.
+ */
+aarch64_sve_change_el(env, cur_el, new_el, is_a64(env));
if (cur_el < new_el) {
/* Entry vector offset depends on whether the implemented EL
@@ -12791,9 +12795,11 @@ void aarch64_sve_narrow_vq(CPUARMState *env, unsigned
vq)
/*
* Notice a change in SVE vector size when changing EL.
*/
-void aarch64_sve_change_el(CPUARMState *env, int old_el, int new_el)
+void aarch64_sve_change_el(CPUARMState *env, int old_el,
+ int new_el, bool el0_a64)
{
int old_len, new_len;
+bool old_a64, new_a64;
/* Nothing to do if no SVE. */
if (!arm_feature(env, ARM_FEATURE_SVE)) {
@@ -12817,9 +12823,11 @@ void aarch64_sve_change_el(CPUARMState *env, int
old_el, int new_el)
* we already have the correct register contents when encountering the
* vq0->vq0 transition between EL0->EL1.
*/
-old_len = (arm_el_is_aa64(env, old_el) && !sve_exception_el(env, old_el)
+old_a64 = old_el ? arm_el_is_aa64(env, old_el) : el0_a64;
+old_len = (old_a64 && !sve_exception_el(env, old_el)
? sve_zcr_len_for_el(env, old_el) : 0);
-new_len = (arm_el_is_aa64(env, new_el) && !sve_exception_el(env, new_el)
+new_a64 = new_el ? arm_el_is_aa64(env, new_el) : el0_a64;
+new_len = (new_a64 && !sve_exception_el(env, new_el)
? sve_zcr_len_for_el(env, new_el) : 0);
/* When changing vector length, clear inaccessible state. */
diff --git a/target/arm/op_helper.c b/target/arm/op_helper.c
index fb15a13e6c..d915579712 100644
--- a/target/arm/op_helper.c
+++ b/target/arm/op_helper.c
@@ -1101,7 +1101,11 @@ void HELPER(exception_return)(CPUARMState *env)
"AArch64 EL%d PC 0x%" PRIx64 "\n",
cur_el, new_el, env->pc);
}
-aarch64_sve_change_el(env, cur_el, new_el);
+/*
+ * Note that cur_el can never be 0. If new_el is 0, then
+ * el0_a64 is return_to_aa64, else el0_a64 is ignored.
+ */
+aarch64_sve_change_el(env, cur_el, new_el, return_to_aa64);
qemu_mutex_lock_iothread();
arm_call_el_change_hook(arm_env_get_cpu(env));
--
2.17.1
[Qemu-devel] [PATCH v3 02/10] target/arm: Define fields of ISAR registers
Reviewed-by: Philippe Mathieu-Daudé Signed-off-by: Richard Henderson --- target/arm/cpu.h | 88 1 file changed, 88 insertions(+) diff --git a/target/arm/cpu.h b/target/arm/cpu.h index 54362ddce8..f00c0444c4 100644 --- a/target/arm/cpu.h +++ b/target/arm/cpu.h @@ -1443,6 +1443,94 @@ FIELD(V7M_CSSELR, LEVEL, 1, 3) */ FIELD(V7M_CSSELR, INDEX, 0, 4) +/* + * System register ID fields. + */ +FIELD(ID_ISAR0, SWAP, 0, 4) +FIELD(ID_ISAR0, BITCOUNT, 4, 4) +FIELD(ID_ISAR0, BITFIELD, 8, 4) +FIELD(ID_ISAR0, CMPBRANCH, 12, 4) +FIELD(ID_ISAR0, COPROC, 16, 4) +FIELD(ID_ISAR0, DEBUG, 20, 4) +FIELD(ID_ISAR0, DIVIDE, 24, 4) + +FIELD(ID_ISAR1, ENDIAN, 0, 4) +FIELD(ID_ISAR1, EXCEPT, 4, 4) +FIELD(ID_ISAR1, EXCEPT_AR, 8, 4) +FIELD(ID_ISAR1, EXTEND, 12, 4) +FIELD(ID_ISAR1, IFTHEN, 16, 4) +FIELD(ID_ISAR1, IMMEDIATE, 20, 4) +FIELD(ID_ISAR1, INTERWORK, 24, 4) +FIELD(ID_ISAR1, JAZELLE, 28, 4) + +FIELD(ID_ISAR2, LOADSTORE, 0, 4) +FIELD(ID_ISAR2, MEMHINT, 4, 4) +FIELD(ID_ISAR2, MULTIACCESSINT, 8, 4) +FIELD(ID_ISAR2, MULT, 12, 4) +FIELD(ID_ISAR2, MULTS, 16, 4) +FIELD(ID_ISAR2, MULTU, 20, 4) +FIELD(ID_ISAR2, PSR_AR, 24, 4) +FIELD(ID_ISAR2, REVERSAL, 28, 4) + +FIELD(ID_ISAR3, SATURATE, 0, 4) +FIELD(ID_ISAR3, SIMD, 4, 4) +FIELD(ID_ISAR3, SVC, 8, 4) +FIELD(ID_ISAR3, SYNCHPRIM, 12, 4) +FIELD(ID_ISAR3, TABBRANCH, 16, 4) +FIELD(ID_ISAR3, T32COPY, 20, 4) +FIELD(ID_ISAR3, TRUENOP, 24, 4) +FIELD(ID_ISAR3, T32EE, 28, 4) + +FIELD(ID_ISAR4, UNPRIV, 0, 4) +FIELD(ID_ISAR4, WITHSHIFTS, 4, 4) +FIELD(ID_ISAR4, WRITEBACK, 8, 4) +FIELD(ID_ISAR4, SMC, 12, 4) +FIELD(ID_ISAR4, BARRIER, 16, 4) +FIELD(ID_ISAR4, SYNCHPRIM_FRAC, 20, 4) +FIELD(ID_ISAR4, PSR_M, 24, 4) +FIELD(ID_ISAR4, SWP_FRAC, 28, 4) + +FIELD(ID_ISAR5, SEVL, 0, 4) +FIELD(ID_ISAR5, AES, 4, 4) +FIELD(ID_ISAR5, SHA1, 8, 4) +FIELD(ID_ISAR5, SHA2, 12, 4) +FIELD(ID_ISAR5, CRC32, 16, 4) +FIELD(ID_ISAR5, RDM, 24, 4) +FIELD(ID_ISAR5, VCMA, 28, 4) + +FIELD(ID_ISAR6, JSCVT, 0, 4) +FIELD(ID_ISAR6, DP, 4, 4) +FIELD(ID_ISAR6, FHM, 8, 4) +FIELD(ID_ISAR6, SB, 12, 4) +FIELD(ID_ISAR6, SPECRES, 16, 4) + +FIELD(ID_AA64ISAR0, AES, 4, 4) +FIELD(ID_AA64ISAR0, SHA1, 8, 4) +FIELD(ID_AA64ISAR0, SHA2, 12, 4) +FIELD(ID_AA64ISAR0, CRC32, 16, 4) +FIELD(ID_AA64ISAR0, ATOMIC, 20, 4) +FIELD(ID_AA64ISAR0, RDM, 28, 4) +FIELD(ID_AA64ISAR0, SHA3, 32, 4) +FIELD(ID_AA64ISAR0, SM3, 36, 4) +FIELD(ID_AA64ISAR0, SM4, 40, 4) +FIELD(ID_AA64ISAR0, DP, 44, 4) +FIELD(ID_AA64ISAR0, FHM, 48, 4) +FIELD(ID_AA64ISAR0, TS, 52, 4) +FIELD(ID_AA64ISAR0, TLB, 56, 4) +FIELD(ID_AA64ISAR0, RNDR, 60, 4) + +FIELD(ID_AA64ISAR1, DPB, 0, 4) +FIELD(ID_AA64ISAR1, APA, 4, 4) +FIELD(ID_AA64ISAR1, API, 8, 4) +FIELD(ID_AA64ISAR1, JSCVT, 12, 4) +FIELD(ID_AA64ISAR1, FCMA, 16, 4) +FIELD(ID_AA64ISAR1, LRCPC, 20, 4) +FIELD(ID_AA64ISAR1, GPA, 24, 4) +FIELD(ID_AA64ISAR1, GPI, 28, 4) +FIELD(ID_AA64ISAR1, FRINTTS, 32, 4) +FIELD(ID_AA64ISAR1, SB, 36, 4) +FIELD(ID_AA64ISAR1, SPECRES, 40, 4) + QEMU_BUILD_BUG_ON(ARRAY_SIZE(((ARMCPU *)0)->ccsidr) <= R_V7M_CSSELR_INDEX_MASK); /* If adding a feature bit which corresponds to a Linux ELF -- 2.17.1
Re: [Qemu-devel] [PATCH v1 10/12] target/arm: Add the Cortex-A72
On Mon, Oct 08, 2018 at 02:10:29PM +0100, Peter Maydell wrote:
> On 3 October 2018 at 16:07, Edgar E. Iglesias
> wrote:
> > From: "Edgar E. Iglesias"
> >
> > Add the ARM Cortex-A72.
> >
> > Signed-off-by: Edgar E. Iglesias
> > ---
> > target/arm/cpu64.c | 59
> > ++
> > 1 file changed, 59 insertions(+)
> >
> > diff --git a/target/arm/cpu64.c b/target/arm/cpu64.c
> > index 800bff7..02e500b 100644
> > --- a/target/arm/cpu64.c
> > +++ b/target/arm/cpu64.c
> > @@ -218,6 +218,64 @@ static void aarch64_a53_initfn(Object *obj)
> > define_arm_cp_regs(cpu, cortex_a57_a53_cp_reginfo);
> > }
> >
> > +static void aarch64_a72_initfn(Object *obj)
> > +{
> > +ARMCPU *cpu = ARM_CPU(obj);
> > +
> > +cpu->dtb_compatible = "arm,cortex-a72";
> > +set_feature(>env, ARM_FEATURE_V8);
> > +set_feature(>env, ARM_FEATURE_VFP4);
> > +set_feature(>env, ARM_FEATURE_NEON);
> > +set_feature(>env, ARM_FEATURE_GENERIC_TIMER);
> > +set_feature(>env, ARM_FEATURE_AARCH64);
> > +set_feature(>env, ARM_FEATURE_CBAR_RO);
> > +set_feature(>env, ARM_FEATURE_V8_AES);
> > +set_feature(>env, ARM_FEATURE_V8_SHA1);
> > +set_feature(>env, ARM_FEATURE_V8_SHA256);
> > +set_feature(>env, ARM_FEATURE_V8_PMULL);
> > +set_feature(>env, ARM_FEATURE_CRC);
> > +set_feature(>env, ARM_FEATURE_EL2);
> > +set_feature(>env, ARM_FEATURE_EL3);
>
> No ARM_FEATURE_PMU ?
Will add in v2.
>
> > +cpu->midr = 0x410fd083;
> > +cpu->revidr = 0x;
> > +cpu->reset_fpsid = 0x41034080;
> > +cpu->mvfr0 = 0x10110222;
> > +cpu->mvfr1 = 0x1211;
> > +cpu->mvfr2 = 0x0043;
> > +cpu->ctr = 0x8444c004;
> > +cpu->reset_sctlr = 0x00c50838;
>
> Do you happen to have the hardware to hand to check what the
> top 4 bits of the reset value of SCTLR_ELx are? I think they
> should be 0x3 -- the Arm ARM says that [29:28] are RES1 (as
> does the A72 TRM, though its top level summary table lists
> 0x00c50838 as the reset value for some of the SCTLR_ELx.)
>
> QEMU may have the wrong value for A53/A57 here too, I suspect.
I don't have access to the A72s at the moment but looking at logs
it seems to be 0x00c50838 for both the A53 and A72.
Looking at "Table 4-118 SCTLR bit assignments" in the A72 TRM,
bits [30:28] seem to have been allocated. Bit 30 depends on
configuration inputs to the core and [29:28] seem to be hard-coded
to zero.
On ZynqMP and Versal, bit 30 (CFGTE) seems to be runtime configurable
prior to releasing the A53s/A72s from reset. I haven't tried it though.
I ran a test on the ZynqMP a53s and got the folloing right after reset,
for EL1, EL2 and EL3:
sctlr c52838 30c50838 c52838
The 0x2000 mask is due to V-hivecs being set.
SCLTR_EL2 seems to get the RES1 bits set to 1...
Perhaps we should just leave it as is for now?
>
> > +cpu->id_pfr0 = 0x0131;
> > +cpu->id_pfr1 = 0x00011011;
> > +cpu->id_dfr0 = 0x03010066;
> > +cpu->id_afr0 = 0x;
> > +cpu->id_mmfr0 = 0x10201105;
> > +cpu->id_mmfr1 = 0x4000;
> > +cpu->id_mmfr2 = 0x0126;
> > +cpu->id_mmfr3 = 0x02102211;
> > +cpu->id_isar0 = 0x02101110;
> > +cpu->id_isar1 = 0x13112111;
> > +cpu->id_isar2 = 0x21232042;
> > +cpu->id_isar3 = 0x01112131;
> > +cpu->id_isar4 = 0x00011142;
> > +cpu->id_isar5 = 0x00011121;
> > +cpu->id_aa64pfr0 = 0x;
> > +cpu->id_aa64dfr0 = 0x10305106;
> > +cpu->pmceid0 = 0x;
> > +cpu->pmceid1 = 0x;
> > +cpu->id_aa64isar0 = 0x00011120;
> > +cpu->id_aa64mmfr0 = 0x1124;
> > +cpu->dbgdidr = 0x3516d000;
> > +cpu->clidr = 0x0a200023;
> > +cpu->ccsidr[0] = 0x701fe00a; /* 32KB L1 dcache */
> > +cpu->ccsidr[1] = 0x201fe012; /* 48KB L1 icache */
> > +cpu->ccsidr[2] = 0x707fe07a; /* 1MB L2 cache */
> > +cpu->dcz_blocksize = 4; /* 64 bytes */
> > +cpu->gic_num_lrs = 4;
> > +cpu->gic_vpribits = 5;
> > +cpu->gic_vprebits = 5;
> > +define_arm_cp_regs(cpu, cortex_a57_a53_cp_reginfo);
>
> The impdef registers do seem to be basically the same as
> the A53/A57. The function name is now a bit inaccurate,
> though, but I don't have a good idea for a better name.
>
> Otherwise
> Reviewed-by: Peter Maydell
>
> thanks
> -- PMM
[Qemu-devel] [PATCH v3 05/10] target/arm: Fix cortex-a7 id_isar0
The incorrect value advertised only thumb2 div without arm div. Reviewed-by: Philippe Mathieu-Daudé Signed-off-by: Richard Henderson --- target/arm/cpu.c | 5 - 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/target/arm/cpu.c b/target/arm/cpu.c index ac46641541..83a6cb535f 100644 --- a/target/arm/cpu.c +++ b/target/arm/cpu.c @@ -1587,7 +1587,10 @@ static void cortex_a7_initfn(Object *obj) cpu->id_mmfr1 = 0x4000; cpu->id_mmfr2 = 0x0124; cpu->id_mmfr3 = 0x02102211; -cpu->id_isar0 = 0x01101110; +/* a7_mpcore_r0p5_trm, page 4-4 gives 0x01101110; but + * table 4-41 gives 0x02101110, which includes the arm div insns. + */ +cpu->id_isar0 = 0x02101110; cpu->id_isar1 = 0x13112111; cpu->id_isar2 = 0x21232041; cpu->id_isar3 = 0x2131; -- 2.17.1
[Qemu-devel] [PATCH v3 07/10] target/arm: Convert jazelle from feature bit to isar1 test
Having V6 alone imply jazelle was wrong for cortex-m0.
Change to an assertion for V6 & !M.
This was harmless, because the only place we tested ARM_FEATURE_JAZELLE
was for 'bxj' in disas_arm(), which is unreachable for M-profile cores.
Reviewed-by: Philippe Mathieu-Daudé
Signed-off-by: Richard Henderson
---
target/arm/cpu.h | 6 +-
target/arm/translate.h | 1 +
target/arm/cpu.c | 17 ++---
target/arm/translate.c | 2 +-
4 files changed, 21 insertions(+), 5 deletions(-)
diff --git a/target/arm/cpu.h b/target/arm/cpu.h
index 2af971e823..557ef8daf9 100644
--- a/target/arm/cpu.h
+++ b/target/arm/cpu.h
@@ -1580,7 +1580,6 @@ enum arm_features {
ARM_FEATURE_PMU, /* has PMU support */
ARM_FEATURE_VBAR, /* has cp15 VBAR */
ARM_FEATURE_M_SECURITY, /* M profile Security Extension */
-ARM_FEATURE_JAZELLE, /* has (trivial) Jazelle implementation */
ARM_FEATURE_SVE, /* has Scalable Vector Extension */
ARM_FEATURE_V8_FP16, /* implements v8.2 half-precision float */
ARM_FEATURE_M_MAIN, /* M profile Main Extension */
@@ -3147,6 +3146,11 @@ static inline bool aa32_feature_arm_div(ARMCPU *cpu)
return FIELD_EX32(cpu->id_isar0, ID_ISAR0, DIVIDE) > 1;
}
+static inline bool aa32_feature_jazelle(ARMCPU *cpu)
+{
+return FIELD_EX32(cpu->id_isar1, ID_ISAR1, JAZELLE) != 0;
+}
+
static inline bool aa32_feature_aes(ARMCPU *cpu)
{
return FIELD_EX32(cpu->id_isar5, ID_ISAR5, AES) != 0;
diff --git a/target/arm/translate.h b/target/arm/translate.h
index 8279465d1d..bd394bdf69 100644
--- a/target/arm/translate.h
+++ b/target/arm/translate.h
@@ -197,6 +197,7 @@ static inline TCGv_i32 get_ahp_flag(void)
FORWARD_FEATURE(thumb_div)
FORWARD_FEATURE(arm_div)
+FORWARD_FEATURE(jazelle)
FORWARD_FEATURE(aes)
FORWARD_FEATURE(pmull)
FORWARD_FEATURE(sha1)
diff --git a/target/arm/cpu.c b/target/arm/cpu.c
index f068b4c476..4e2609aa7e 100644
--- a/target/arm/cpu.c
+++ b/target/arm/cpu.c
@@ -850,8 +850,8 @@ static void arm_cpu_realizefn(DeviceState *dev, Error
**errp)
}
if (arm_feature(env, ARM_FEATURE_V6)) {
set_feature(env, ARM_FEATURE_V5);
-set_feature(env, ARM_FEATURE_JAZELLE);
if (!arm_feature(env, ARM_FEATURE_M)) {
+assert(aa32_feature_jazelle(cpu));
set_feature(env, ARM_FEATURE_AUXCR);
}
}
@@ -1078,11 +1078,16 @@ static void arm926_initfn(Object *obj)
set_feature(>env, ARM_FEATURE_VFP);
set_feature(>env, ARM_FEATURE_DUMMY_C15_REGS);
set_feature(>env, ARM_FEATURE_CACHE_TEST_CLEAN);
-set_feature(>env, ARM_FEATURE_JAZELLE);
cpu->midr = 0x41069265;
cpu->reset_fpsid = 0x41011090;
cpu->ctr = 0x1dd20d2;
cpu->reset_sctlr = 0x00090078;
+
+/*
+ * ARMv5 does not have the ID_ISAR registers, but we can still
+ * set the field to indicate Jazelle support within QEMU.
+ */
+cpu->id_isar1 = FIELD_DP32(cpu->id_isar1, ID_ISAR1, JAZELLE, 1);
}
static void arm946_initfn(Object *obj)
@@ -1108,12 +1113,18 @@ static void arm1026_initfn(Object *obj)
set_feature(>env, ARM_FEATURE_AUXCR);
set_feature(>env, ARM_FEATURE_DUMMY_C15_REGS);
set_feature(>env, ARM_FEATURE_CACHE_TEST_CLEAN);
-set_feature(>env, ARM_FEATURE_JAZELLE);
cpu->midr = 0x4106a262;
cpu->reset_fpsid = 0x410110a0;
cpu->ctr = 0x1dd20d2;
cpu->reset_sctlr = 0x00090078;
cpu->reset_auxcr = 1;
+
+/*
+ * ARMv5 does not have the ID_ISAR registers, but we can still
+ * set the field to indicate Jazelle support within QEMU.
+ */
+cpu->id_isar1 = FIELD_DP32(cpu->id_isar1, ID_ISAR1, JAZELLE, 1);
+
{
/* The 1026 had an IFAR at c6,c0,0,1 rather than the ARMv6 c6,c0,0,2 */
ARMCPRegInfo ifar = {
diff --git a/target/arm/translate.c b/target/arm/translate.c
index b1ee6533cc..54ecf369cb 100644
--- a/target/arm/translate.c
+++ b/target/arm/translate.c
@@ -42,7 +42,7 @@
#define ENABLE_ARCH_5 arm_dc_feature(s, ARM_FEATURE_V5)
/* currently all emulated v5 cores are also v5TE, so don't bother */
#define ENABLE_ARCH_5TE arm_dc_feature(s, ARM_FEATURE_V5)
-#define ENABLE_ARCH_5Jarm_dc_feature(s, ARM_FEATURE_JAZELLE)
+#define ENABLE_ARCH_5Jaa32_dc_feature_jazelle(s)
#define ENABLE_ARCH_6 arm_dc_feature(s, ARM_FEATURE_V6)
#define ENABLE_ARCH_6Karm_dc_feature(s, ARM_FEATURE_V6K)
#define ENABLE_ARCH_6T2 arm_dc_feature(s, ARM_FEATURE_THUMB2)
--
2.17.1
[Qemu-devel] [PATCH v3 04/10] target/arm: Align cortex-r5 id_isar0
The missing nibble made it more difficult to read. Reviewed-by: Philippe Mathieu-Daudé Signed-off-by: Richard Henderson --- target/arm/cpu.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/target/arm/cpu.c b/target/arm/cpu.c index b7d9942aa3..ac46641541 100644 --- a/target/arm/cpu.c +++ b/target/arm/cpu.c @@ -1397,7 +1397,7 @@ static void cortex_r5_initfn(Object *obj) cpu->id_mmfr1 = 0x; cpu->id_mmfr2 = 0x0120; cpu->id_mmfr3 = 0x0211; -cpu->id_isar0 = 0x210; +cpu->id_isar0 = 0x0210; cpu->id_isar1 = 0x13112111; cpu->id_isar2 = 0x21232141; cpu->id_isar3 = 0x01112131; -- 2.17.1
[Qemu-devel] [PATCH v3 06/10] target/arm: Convert division from feature bits to isar0 tests
Both arm and thumb2 division are controlled by the same ISAR field,
which takes care of the arm implies thumb case. Having M imply
thumb2 division was wrong for cortex-m0, which is v6m and does not
have thumb2 at all, much less thumb2 division.
Reviewed-by: Philippe Mathieu-Daudé
Signed-off-by: Richard Henderson
---
target/arm/cpu.h | 12 ++--
target/arm/translate.h | 2 ++
linux-user/elfload.c | 4 ++--
target/arm/cpu.c | 10 +-
target/arm/translate.c | 4 ++--
5 files changed, 17 insertions(+), 15 deletions(-)
diff --git a/target/arm/cpu.h b/target/arm/cpu.h
index 66eaa40ed9..2af971e823 100644
--- a/target/arm/cpu.h
+++ b/target/arm/cpu.h
@@ -1550,7 +1550,6 @@ enum arm_features {
ARM_FEATURE_VFP3,
ARM_FEATURE_VFP_FP16,
ARM_FEATURE_NEON,
-ARM_FEATURE_THUMB_DIV, /* divide supported in Thumb encoding */
ARM_FEATURE_M, /* Microcontroller profile. */
ARM_FEATURE_OMAPCP, /* OMAP specific CP15 ops handling. */
ARM_FEATURE_THUMB2EE,
@@ -1560,7 +1559,6 @@ enum arm_features {
ARM_FEATURE_V5,
ARM_FEATURE_STRONGARM,
ARM_FEATURE_VAPA, /* cp15 VA to PA lookups */
-ARM_FEATURE_ARM_DIV, /* divide supported in ARM encoding */
ARM_FEATURE_VFP4, /* VFPv4 (implies that NEON is v2) */
ARM_FEATURE_GENERIC_TIMER,
ARM_FEATURE_MVFR, /* Media and VFP Feature Registers 0 and 1 */
@@ -3139,6 +3137,16 @@ extern const uint64_t pred_esz_masks[4];
/*
* 32-bit feature tests via id registers.
*/
+static inline bool aa32_feature_thumb_div(ARMCPU *cpu)
+{
+return FIELD_EX32(cpu->id_isar0, ID_ISAR0, DIVIDE) != 0;
+}
+
+static inline bool aa32_feature_arm_div(ARMCPU *cpu)
+{
+return FIELD_EX32(cpu->id_isar0, ID_ISAR0, DIVIDE) > 1;
+}
+
static inline bool aa32_feature_aes(ARMCPU *cpu)
{
return FIELD_EX32(cpu->id_isar5, ID_ISAR5, AES) != 0;
diff --git a/target/arm/translate.h b/target/arm/translate.h
index 1d60569583..8279465d1d 100644
--- a/target/arm/translate.h
+++ b/target/arm/translate.h
@@ -195,6 +195,8 @@ static inline TCGv_i32 get_ahp_flag(void)
static inline bool aa32_dc_feature_##NAME(DisasContext *dc) \
{ return aa32_feature_##NAME(dc->cpu); }
+FORWARD_FEATURE(thumb_div)
+FORWARD_FEATURE(arm_div)
FORWARD_FEATURE(aes)
FORWARD_FEATURE(pmull)
FORWARD_FEATURE(sha1)
diff --git a/linux-user/elfload.c b/linux-user/elfload.c
index 571beca8fd..9b00e977d8 100644
--- a/linux-user/elfload.c
+++ b/linux-user/elfload.c
@@ -471,8 +471,8 @@ static uint32_t get_elf_hwcap(void)
GET_FEATURE(ARM_FEATURE_VFP3, ARM_HWCAP_ARM_VFPv3);
GET_FEATURE(ARM_FEATURE_V6K, ARM_HWCAP_ARM_TLS);
GET_FEATURE(ARM_FEATURE_VFP4, ARM_HWCAP_ARM_VFPv4);
-GET_FEATURE(ARM_FEATURE_ARM_DIV, ARM_HWCAP_ARM_IDIVA);
-GET_FEATURE(ARM_FEATURE_THUMB_DIV, ARM_HWCAP_ARM_IDIVT);
+GET_FEATURE_ID(arm_div, ARM_HWCAP_ARM_IDIVA);
+GET_FEATURE_ID(thumb_div, ARM_HWCAP_ARM_IDIVT);
/* All QEMU's VFPv3 CPUs have 32 registers, see VFP_DREG in translate.c.
* Note that the ARM_HWCAP_ARM_VFPv3D16 bit is always the inverse of
* ARM_HWCAP_ARM_VFPD32 (and so always clear for QEMU); it is unrelated
diff --git a/target/arm/cpu.c b/target/arm/cpu.c
index 83a6cb535f..f068b4c476 100644
--- a/target/arm/cpu.c
+++ b/target/arm/cpu.c
@@ -825,7 +825,7 @@ static void arm_cpu_realizefn(DeviceState *dev, Error
**errp)
* Presence of EL2 itself is ARM_FEATURE_EL2, and of the
* Security Extensions is ARM_FEATURE_EL3.
*/
-set_feature(env, ARM_FEATURE_ARM_DIV);
+assert(aa32_feature_arm_div(cpu));
set_feature(env, ARM_FEATURE_LPAE);
set_feature(env, ARM_FEATURE_V7);
}
@@ -858,12 +858,6 @@ static void arm_cpu_realizefn(DeviceState *dev, Error
**errp)
if (arm_feature(env, ARM_FEATURE_V5)) {
set_feature(env, ARM_FEATURE_V4T);
}
-if (arm_feature(env, ARM_FEATURE_M)) {
-set_feature(env, ARM_FEATURE_THUMB_DIV);
-}
-if (arm_feature(env, ARM_FEATURE_ARM_DIV)) {
-set_feature(env, ARM_FEATURE_THUMB_DIV);
-}
if (arm_feature(env, ARM_FEATURE_VFP4)) {
set_feature(env, ARM_FEATURE_VFP3);
set_feature(env, ARM_FEATURE_VFP_FP16);
@@ -1384,8 +1378,6 @@ static void cortex_r5_initfn(Object *obj)
ARMCPU *cpu = ARM_CPU(obj);
set_feature(>env, ARM_FEATURE_V7);
-set_feature(>env, ARM_FEATURE_THUMB_DIV);
-set_feature(>env, ARM_FEATURE_ARM_DIV);
set_feature(>env, ARM_FEATURE_V7MP);
set_feature(>env, ARM_FEATURE_PMSA);
cpu->midr = 0x411fc153; /* r1p3 */
diff --git a/target/arm/translate.c b/target/arm/translate.c
index ea8545c43b..b1ee6533cc 100644
--- a/target/arm/translate.c
+++ b/target/arm/translate.c
@@ -9755,7 +9755,7 @@ static void disas_arm_insn(DisasContext *s, unsigned int
insn)
case 1:
case 3:
/* SDIV, UDIV */
-if (!arm_dc_feature(s, ARM_FEATURE_ARM_DIV)) {
+
[Qemu-devel] [PATCH v3 09/10] target/arm: Convert sve from feature bit to aa64pfr0 test
Reviewed-by: Philippe Mathieu-Daudé
Signed-off-by: Richard Henderson
---
target/arm/cpu.h| 16 +++-
target/arm/translate-a64.h | 1 +
linux-user/aarch64/signal.c | 4 ++--
linux-user/elfload.c| 2 +-
linux-user/syscall.c| 10 ++
target/arm/cpu64.c | 5 -
target/arm/helper.c | 9 ++---
target/arm/machine.c| 3 +--
target/arm/translate-a64.c | 4 ++--
9 files changed, 38 insertions(+), 16 deletions(-)
diff --git a/target/arm/cpu.h b/target/arm/cpu.h
index d8cb9633d2..a97b471fff 100644
--- a/target/arm/cpu.h
+++ b/target/arm/cpu.h
@@ -1531,6 +1531,16 @@ FIELD(ID_AA64ISAR1, FRINTTS, 32, 4)
FIELD(ID_AA64ISAR1, SB, 36, 4)
FIELD(ID_AA64ISAR1, SPECRES, 40, 4)
+FIELD(ID_AA64PFR0, EL0, 0, 4)
+FIELD(ID_AA64PFR0, EL1, 4, 4)
+FIELD(ID_AA64PFR0, EL2, 8, 4)
+FIELD(ID_AA64PFR0, EL3, 12, 4)
+FIELD(ID_AA64PFR0, FP, 16, 4)
+FIELD(ID_AA64PFR0, ADVSIMD, 20, 4)
+FIELD(ID_AA64PFR0, GIC, 24, 4)
+FIELD(ID_AA64PFR0, RAS, 28, 4)
+FIELD(ID_AA64PFR0, SVE, 32, 4)
+
QEMU_BUILD_BUG_ON(ARRAY_SIZE(((ARMCPU *)0)->ccsidr) <=
R_V7M_CSSELR_INDEX_MASK);
/* If adding a feature bit which corresponds to a Linux ELF
@@ -1579,7 +1589,6 @@ enum arm_features {
ARM_FEATURE_PMU, /* has PMU support */
ARM_FEATURE_VBAR, /* has cp15 VBAR */
ARM_FEATURE_M_SECURITY, /* M profile Security Extension */
-ARM_FEATURE_SVE, /* has Scalable Vector Extension */
ARM_FEATURE_V8_FP16, /* implements v8.2 half-precision float */
ARM_FEATURE_M_MAIN, /* M profile Main Extension */
};
@@ -3263,4 +3272,9 @@ static inline bool aa64_feature_fcma(ARMCPU *cpu)
return FIELD_EX64(cpu->id_aa64isar1, ID_AA64ISAR1, FCMA) != 0;
}
+static inline bool aa64_feature_sve(ARMCPU *cpu)
+{
+return FIELD_EX64(cpu->id_aa64pfr0, ID_AA64PFR0, SVE) != 0;
+}
+
#endif
diff --git a/target/arm/translate-a64.h b/target/arm/translate-a64.h
index b4ef9eb024..636f3fded3 100644
--- a/target/arm/translate-a64.h
+++ b/target/arm/translate-a64.h
@@ -140,6 +140,7 @@ FORWARD_FEATURE(sm3)
FORWARD_FEATURE(sm4)
FORWARD_FEATURE(dp)
FORWARD_FEATURE(fcma)
+FORWARD_FEATURE(sve)
#undef FORWARD_FEATURE
diff --git a/linux-user/aarch64/signal.c b/linux-user/aarch64/signal.c
index 07fedfc33c..65272fb7a9 100644
--- a/linux-user/aarch64/signal.c
+++ b/linux-user/aarch64/signal.c
@@ -314,7 +314,7 @@ static int target_restore_sigframe(CPUARMState *env,
break;
case TARGET_SVE_MAGIC:
-if (arm_feature(env, ARM_FEATURE_SVE)) {
+if (aa64_feature_sve(arm_env_get_cpu(env))) {
vq = (env->vfp.zcr_el[1] & 0xf) + 1;
sve_size = QEMU_ALIGN_UP(TARGET_SVE_SIG_CONTEXT_SIZE(vq), 16);
if (!sve && size == sve_size) {
@@ -433,7 +433,7 @@ static void target_setup_frame(int usig, struct
target_sigaction *ka,
);
/* SVE state needs saving only if it exists. */
-if (arm_feature(env, ARM_FEATURE_SVE)) {
+if (aa64_feature_sve(arm_env_get_cpu(env))) {
vq = (env->vfp.zcr_el[1] & 0xf) + 1;
sve_size = QEMU_ALIGN_UP(TARGET_SVE_SIG_CONTEXT_SIZE(vq), 16);
sve_ofs = alloc_sigframe_space(sve_size, );
diff --git a/linux-user/elfload.c b/linux-user/elfload.c
index 3061d703b2..e3585f4cb6 100644
--- a/linux-user/elfload.c
+++ b/linux-user/elfload.c
@@ -593,7 +593,7 @@ static uint32_t get_elf_hwcap(void)
GET_FEATURE_ID(rdm, ARM_HWCAP_A64_ASIMDRDM);
GET_FEATURE_ID(dp, ARM_HWCAP_A64_ASIMDDP);
GET_FEATURE_ID(fcma, ARM_HWCAP_A64_FCMA);
-GET_FEATURE(ARM_FEATURE_SVE, ARM_HWCAP_A64_SVE);
+GET_FEATURE_ID(sve, ARM_HWCAP_A64_SVE);
#undef GET_FEATURE
#undef GET_FEATURE_ID
diff --git a/linux-user/syscall.c b/linux-user/syscall.c
index ae3c0dfef7..37f315ad4a 100644
--- a/linux-user/syscall.c
+++ b/linux-user/syscall.c
@@ -9356,7 +9356,7 @@ static abi_long do_syscall1(void *cpu_env, int num,
abi_long arg1,
* even though the current architectural maximum is VQ=16.
*/
ret = -TARGET_EINVAL;
-if (arm_feature(cpu_env, ARM_FEATURE_SVE)
+if (aa64_feature_sve(arm_env_get_cpu(cpu_env))
&& arg2 >= 0 && arg2 <= 512 * 16 && !(arg2 & 15)) {
CPUARMState *env = cpu_env;
ARMCPU *cpu = arm_env_get_cpu(env);
@@ -9375,9 +9375,11 @@ static abi_long do_syscall1(void *cpu_env, int num,
abi_long arg1,
return ret;
case TARGET_PR_SVE_GET_VL:
ret = -TARGET_EINVAL;
-if (arm_feature(cpu_env, ARM_FEATURE_SVE)) {
-CPUARMState *env = cpu_env;
-ret = ((env->vfp.zcr_el[1] & 0xf) + 1) * 16;
+{
+ARMCPU *cpu = arm_env_get_cpu(cpu_env);
+if (aa64_feature_sve(cpu)) {
+ret = ((cpu->env.vfp.zcr_el[1] & 0xf) + 1) * 16;
+}
}
return ret;
#endif /* AARCH64 */
diff
[Qemu-devel] [PATCH v3 08/10] target/arm: Convert t32ee from feature bit to isar3 test
Reviewed-by: Philippe Mathieu-Daudé
Signed-off-by: Richard Henderson
---
target/arm/cpu.h | 6 +-
linux-user/elfload.c | 2 +-
target/arm/cpu.c | 4
target/arm/helper.c | 2 +-
target/arm/machine.c | 3 +--
5 files changed, 8 insertions(+), 9 deletions(-)
diff --git a/target/arm/cpu.h b/target/arm/cpu.h
index 557ef8daf9..d8cb9633d2 100644
--- a/target/arm/cpu.h
+++ b/target/arm/cpu.h
@@ -1552,7 +1552,6 @@ enum arm_features {
ARM_FEATURE_NEON,
ARM_FEATURE_M, /* Microcontroller profile. */
ARM_FEATURE_OMAPCP, /* OMAP specific CP15 ops handling. */
-ARM_FEATURE_THUMB2EE,
ARM_FEATURE_V7MP,/* v7 Multiprocessing Extensions */
ARM_FEATURE_V7VE, /* v7 Virtualization Extensions (non-EL2 parts) */
ARM_FEATURE_V4T,
@@ -3151,6 +3150,11 @@ static inline bool aa32_feature_jazelle(ARMCPU *cpu)
return FIELD_EX32(cpu->id_isar1, ID_ISAR1, JAZELLE) != 0;
}
+static inline bool aa32_feature_t32ee(ARMCPU *cpu)
+{
+return FIELD_EX32(cpu->id_isar3, ID_ISAR3, T32EE) != 0;
+}
+
static inline bool aa32_feature_aes(ARMCPU *cpu)
{
return FIELD_EX32(cpu->id_isar5, ID_ISAR5, AES) != 0;
diff --git a/linux-user/elfload.c b/linux-user/elfload.c
index 9b00e977d8..3061d703b2 100644
--- a/linux-user/elfload.c
+++ b/linux-user/elfload.c
@@ -466,7 +466,7 @@ static uint32_t get_elf_hwcap(void)
GET_FEATURE(ARM_FEATURE_V5, ARM_HWCAP_ARM_EDSP);
GET_FEATURE(ARM_FEATURE_VFP, ARM_HWCAP_ARM_VFP);
GET_FEATURE(ARM_FEATURE_IWMMXT, ARM_HWCAP_ARM_IWMMXT);
-GET_FEATURE(ARM_FEATURE_THUMB2EE, ARM_HWCAP_ARM_THUMBEE);
+GET_FEATURE_ID(t32ee, ARM_HWCAP_ARM_THUMBEE);
GET_FEATURE(ARM_FEATURE_NEON, ARM_HWCAP_ARM_NEON);
GET_FEATURE(ARM_FEATURE_VFP3, ARM_HWCAP_ARM_VFPv3);
GET_FEATURE(ARM_FEATURE_V6K, ARM_HWCAP_ARM_TLS);
diff --git a/target/arm/cpu.c b/target/arm/cpu.c
index 4e2609aa7e..f8faea7933 100644
--- a/target/arm/cpu.c
+++ b/target/arm/cpu.c
@@ -1436,7 +1436,6 @@ static void cortex_a8_initfn(Object *obj)
set_feature(>env, ARM_FEATURE_V7);
set_feature(>env, ARM_FEATURE_VFP3);
set_feature(>env, ARM_FEATURE_NEON);
-set_feature(>env, ARM_FEATURE_THUMB2EE);
set_feature(>env, ARM_FEATURE_DUMMY_C15_REGS);
set_feature(>env, ARM_FEATURE_EL3);
cpu->midr = 0x410fc080;
@@ -1505,7 +1504,6 @@ static void cortex_a9_initfn(Object *obj)
set_feature(>env, ARM_FEATURE_VFP3);
set_feature(>env, ARM_FEATURE_VFP_FP16);
set_feature(>env, ARM_FEATURE_NEON);
-set_feature(>env, ARM_FEATURE_THUMB2EE);
set_feature(>env, ARM_FEATURE_EL3);
/* Note that A9 supports the MP extensions even for
* A9UP and single-core A9MP (which are both different
@@ -1568,7 +1566,6 @@ static void cortex_a7_initfn(Object *obj)
set_feature(>env, ARM_FEATURE_V7VE);
set_feature(>env, ARM_FEATURE_VFP4);
set_feature(>env, ARM_FEATURE_NEON);
-set_feature(>env, ARM_FEATURE_THUMB2EE);
set_feature(>env, ARM_FEATURE_GENERIC_TIMER);
set_feature(>env, ARM_FEATURE_DUMMY_C15_REGS);
set_feature(>env, ARM_FEATURE_CBAR_RO);
@@ -1614,7 +1611,6 @@ static void cortex_a15_initfn(Object *obj)
set_feature(>env, ARM_FEATURE_V7VE);
set_feature(>env, ARM_FEATURE_VFP4);
set_feature(>env, ARM_FEATURE_NEON);
-set_feature(>env, ARM_FEATURE_THUMB2EE);
set_feature(>env, ARM_FEATURE_GENERIC_TIMER);
set_feature(>env, ARM_FEATURE_DUMMY_C15_REGS);
set_feature(>env, ARM_FEATURE_CBAR_RO);
diff --git a/target/arm/helper.c b/target/arm/helper.c
index 0efbb5c76c..0da13175be 100644
--- a/target/arm/helper.c
+++ b/target/arm/helper.c
@@ -5356,7 +5356,7 @@ void register_cp_regs_for_features(ARMCPU *cpu)
define_arm_cp_regs(cpu, vmsa_pmsa_cp_reginfo);
define_arm_cp_regs(cpu, vmsa_cp_reginfo);
}
-if (arm_feature(env, ARM_FEATURE_THUMB2EE)) {
+if (aa32_feature_t32ee(cpu)) {
define_arm_cp_regs(cpu, t2ee_cp_reginfo);
}
if (arm_feature(env, ARM_FEATURE_GENERIC_TIMER)) {
diff --git a/target/arm/machine.c b/target/arm/machine.c
index ff4ec22bf7..d44e891533 100644
--- a/target/arm/machine.c
+++ b/target/arm/machine.c
@@ -301,9 +301,8 @@ static const VMStateDescription vmstate_m = {
static bool thumb2ee_needed(void *opaque)
{
ARMCPU *cpu = opaque;
-CPUARMState *env = >env;
-return arm_feature(env, ARM_FEATURE_THUMB2EE);
+return aa32_feature_t32ee(cpu);
}
static const VMStateDescription vmstate_thumb2ee = {
--
2.17.1
[Qemu-devel] [PATCH v3 10/10] target/arm: Convert v8.2-fp16 from feature bit to aa64pfr0 test
Reviewed-by: Philippe Mathieu-Daudé
Signed-off-by: Richard Henderson
---
target/arm/cpu.h | 17 +++-
target/arm/translate-a64.h | 1 +
target/arm/translate.h | 1 +
linux-user/elfload.c | 6 +-
target/arm/cpu64.c | 9 ++---
target/arm/helper.c| 2 +-
target/arm/translate-a64.c | 40 +++---
target/arm/translate.c | 6 +++---
8 files changed, 45 insertions(+), 37 deletions(-)
diff --git a/target/arm/cpu.h b/target/arm/cpu.h
index a97b471fff..1c880b0c29 100644
--- a/target/arm/cpu.h
+++ b/target/arm/cpu.h
@@ -1589,7 +1589,6 @@ enum arm_features {
ARM_FEATURE_PMU, /* has PMU support */
ARM_FEATURE_VBAR, /* has cp15 VBAR */
ARM_FEATURE_M_SECURITY, /* M profile Security Extension */
-ARM_FEATURE_V8_FP16, /* implements v8.2 half-precision float */
ARM_FEATURE_M_MAIN, /* M profile Main Extension */
};
@@ -3204,6 +3203,16 @@ static inline bool aa32_feature_dp(ARMCPU *cpu)
return FIELD_EX32(cpu->id_isar6, ID_ISAR6, DP) != 0;
}
+static inline bool aa32_feature_fp16_arith(ARMCPU *cpu)
+{
+/*
+ * This is a placeholder for use by VCMA until the rest of
+ * the ARMv8.2-FP16 extension is implemented for aa32 mode.
+ * At which point we can properly set and check MVFR1.FPHP.
+ */
+return FIELD_EX64(cpu->id_aa64pfr0, ID_AA64PFR0, FP) == 1;
+}
+
/*
* 64-bit feature tests via id registers.
*/
@@ -3272,6 +3281,12 @@ static inline bool aa64_feature_fcma(ARMCPU *cpu)
return FIELD_EX64(cpu->id_aa64isar1, ID_AA64ISAR1, FCMA) != 0;
}
+static inline bool aa64_feature_fp16(ARMCPU *cpu)
+{
+/* We always set the AdvSIMD and FP fields identically wrt FP16. */
+return FIELD_EX64(cpu->id_aa64pfr0, ID_AA64PFR0, FP) == 1;
+}
+
static inline bool aa64_feature_sve(ARMCPU *cpu)
{
return FIELD_EX64(cpu->id_aa64pfr0, ID_AA64PFR0, SVE) != 0;
diff --git a/target/arm/translate-a64.h b/target/arm/translate-a64.h
index 636f3fded3..e122cef242 100644
--- a/target/arm/translate-a64.h
+++ b/target/arm/translate-a64.h
@@ -140,6 +140,7 @@ FORWARD_FEATURE(sm3)
FORWARD_FEATURE(sm4)
FORWARD_FEATURE(dp)
FORWARD_FEATURE(fcma)
+FORWARD_FEATURE(fp16)
FORWARD_FEATURE(sve)
#undef FORWARD_FEATURE
diff --git a/target/arm/translate.h b/target/arm/translate.h
index bd394bdf69..ad911de98c 100644
--- a/target/arm/translate.h
+++ b/target/arm/translate.h
@@ -206,6 +206,7 @@ FORWARD_FEATURE(crc32)
FORWARD_FEATURE(rdm)
FORWARD_FEATURE(vcma)
FORWARD_FEATURE(dp)
+FORWARD_FEATURE(fp16_arith)
#undef FORWARD_FEATURE
diff --git a/linux-user/elfload.c b/linux-user/elfload.c
index e3585f4cb6..d041ef9d49 100644
--- a/linux-user/elfload.c
+++ b/linux-user/elfload.c
@@ -573,8 +573,6 @@ static uint32_t get_elf_hwcap(void)
hwcaps |= ARM_HWCAP_A64_ASIMD;
/* probe for the extra features */
-#define GET_FEATURE(feat, hwcap) \
-do { if (arm_feature(>env, feat)) { hwcaps |= hwcap; } } while (0)
#define GET_FEATURE_ID(feat, hwcap) \
do { if (aa64_feature_##feat(cpu)) { hwcaps |= hwcap; } } while (0)
@@ -587,15 +585,13 @@ static uint32_t get_elf_hwcap(void)
GET_FEATURE_ID(sha3, ARM_HWCAP_A64_SHA3);
GET_FEATURE_ID(sm3, ARM_HWCAP_A64_SM3);
GET_FEATURE_ID(sm4, ARM_HWCAP_A64_SM4);
-GET_FEATURE(ARM_FEATURE_V8_FP16,
-ARM_HWCAP_A64_FPHP | ARM_HWCAP_A64_ASIMDHP);
+GET_FEATURE_ID(fp16, ARM_HWCAP_A64_FPHP | ARM_HWCAP_A64_ASIMDHP);
GET_FEATURE_ID(atomics, ARM_HWCAP_A64_ATOMICS);
GET_FEATURE_ID(rdm, ARM_HWCAP_A64_ASIMDRDM);
GET_FEATURE_ID(dp, ARM_HWCAP_A64_ASIMDDP);
GET_FEATURE_ID(fcma, ARM_HWCAP_A64_FCMA);
GET_FEATURE_ID(sve, ARM_HWCAP_A64_SVE);
-#undef GET_FEATURE
#undef GET_FEATURE_ID
return hwcaps;
diff --git a/target/arm/cpu64.c b/target/arm/cpu64.c
index ee2c04a627..38e9afef3b 100644
--- a/target/arm/cpu64.c
+++ b/target/arm/cpu64.c
@@ -266,6 +266,8 @@ static void aarch64_max_initfn(Object *obj)
t = cpu->id_aa64pfr0;
t = FIELD_DP64(t, ID_AA64PFR0, SVE, 1);
+t = FIELD_DP64(t, ID_AA64PFR0, FP, 1);
+t = FIELD_DP64(t, ID_AA64PFR0, ADVSIMD, 1);
cpu->id_aa64pfr0 = t;
/* Replicate the same data to the 32-bit id registers. */
@@ -283,13 +285,6 @@ static void aarch64_max_initfn(Object *obj)
cpu->id_isar6 = u;
#ifdef CONFIG_USER_ONLY
-/* We don't set these in system emulation mode for the moment,
- * since we don't correctly set the ID registers to advertise them,
- * and in some cases they're only available in AArch64 and not AArch32,
- * whereas the architecture requires them to be present in both if
- * present in either.
- */
-set_feature(>env, ARM_FEATURE_V8_FP16);
/* For usermode -cpu max we can use a larger and more efficient DCZ
* blocksize since we don't have to follow what the hardware does.
*/
diff --git a/target/arm/helper.c b/target/arm/helper.c
Re: [Qemu-devel] [PATCH 30/31] blockdev: Convert drive_new() to Error
On 08.10.18 19:31, Markus Armbruster wrote:
> Calling error_report() from within a a function that takes an Error **
> argument is suspicious. drive_new() does that, and its caller
> drive_init_func() then exit()s.
I'm afraid I don't quite follow you here. There is no function here
that takes an Error ** already and then calls error_report(). There is
however drive_new() that does not take an Error **, consequentially
calls error_report(), and there is its caller drive_init_func() which
does take an Error ** but does not set it.
So while I fully agree with you to make drive_new() take an Error **
(and thus effectively fix drive_init_func()), I don't quite understand
this explanation.
(Furthermore, drive_init_func() does not exit(). It's main() that
exit()s after calling drive_init_func().)
> Its caller main(), via
> qemu_opts_foreach(), is fine with it, but clean it up anyway:
>
> * Convert drive_new() to Error
>
> * Update add_init_drive() to report the error received from
> drive_new().
>
> * Make main() pass _fatal through qemu_opts_foreach(),
> drive_init_func() to drive_new()
>
> * Make default_drive() pass _abort through qemu_opts_foreach(),
> drive_init_func() to drive_new()
>
> Cc: Kevin Wolf
> Cc: Max Reitz
> Signed-off-by: Markus Armbruster
> ---
> blockdev.c| 27 ++-
> device-hotplug.c | 5 -
> include/sysemu/blockdev.h | 3 ++-
> vl.c | 11 ---
> 4 files changed, 24 insertions(+), 22 deletions(-)
>
> diff --git a/blockdev.c b/blockdev.c
> index a8755bd908..574adbcb7f 100644
> --- a/blockdev.c
> +++ b/blockdev.c
> @@ -759,7 +759,8 @@ QemuOptsList qemu_legacy_drive_opts = {
[...]
> @@ -991,7 +992,7 @@ DriveInfo *drive_new(QemuOpts *all_opts,
> BlockInterfaceType block_default_type)
> bs_opts = NULL;
> if (!blk) {
> if (local_err) {
> -error_report_err(local_err);
> +error_propagate(errp, local_err);
> }
Wait, what would be the case where blockdev_init() returns NULL but
*errp remains unse——— oh no.
There is only one case which is someone specified "format=help". Hm. I
suppose you are as unhappy as me about the fact that because of this
drive_new() cannot guarantee that *errp is set in case of an error.
I think it's ""fine"" (*gnashing teeth*) to keep it this way, but it
means that callers need to continue to check the return value and not
*errp alone.
> goto fail;
> } else {
> diff --git a/device-hotplug.c b/device-hotplug.c
> index cd427e2c76..6090d5f1e9 100644
> --- a/device-hotplug.c
> +++ b/device-hotplug.c
> @@ -28,6 +28,7 @@
> #include "sysemu/block-backend.h"
> #include "sysemu/blockdev.h"
> #include "qapi/qmp/qdict.h"
> +#include "qapi/error.h"
> #include "qemu/config-file.h"
> #include "qemu/option.h"
> #include "sysemu/sysemu.h"
> @@ -36,6 +37,7 @@
>
> static DriveInfo *add_init_drive(const char *optstr)
> {
> +Error *err = NULL;
> DriveInfo *dinfo;
> QemuOpts *opts;
> MachineClass *mc;
> @@ -45,8 +47,9 @@ static DriveInfo *add_init_drive(const char *optstr)
> return NULL;
>
> mc = MACHINE_GET_CLASS(current_machine);
> -dinfo = drive_new(opts, mc->block_default_type);
> +dinfo = drive_new(opts, mc->block_default_type, );
> if (!dinfo) {
> +error_report_err(err);
> qemu_opts_del(opts);
> return NULL;
> }
> diff --git a/include/sysemu/blockdev.h b/include/sysemu/blockdev.h
> index 24954b94e0..d34c4920dc 100644
> --- a/include/sysemu/blockdev.h
> +++ b/include/sysemu/blockdev.h
> @@ -54,7 +54,8 @@ DriveInfo *drive_get_next(BlockInterfaceType type);
> QemuOpts *drive_def(const char *optstr);
> QemuOpts *drive_add(BlockInterfaceType type, int index, const char *file,
> const char *optstr);
> -DriveInfo *drive_new(QemuOpts *arg, BlockInterfaceType block_default_type);
> +DriveInfo *drive_new(QemuOpts *arg, BlockInterfaceType block_default_type,
> + Error **errp);
>
> /* device-hotplug */
>
> diff --git a/vl.c b/vl.c
> index 0d25956b2f..101e0123d9 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -1129,7 +1129,7 @@ static int drive_init_func(void *opaque, QemuOpts
> *opts, Error **errp)
> {
> BlockInterfaceType *block_default_type = opaque;
>
> -return drive_new(opts, *block_default_type) == NULL;
> +return drive_new(opts, *block_default_type, errp) == NULL;
> }
>
> static int drive_enable_snapshot(void *opaque, QemuOpts *opts, Error **errp)
> @@ -1155,8 +1155,7 @@ static void default_drive(int enable, int snapshot,
> BlockInterfaceType type,
> drive_enable_snapshot(NULL, opts, NULL);
> }
>
> -dinfo = drive_new(opts, type);
> -assert(dinfo);
> +dinfo = drive_new(opts, type, _abort);
Which means the assertion is still necessary here.
> dinfo->is_default = true;
>
> }
> @@ -4348,10 +4347,8 @@ int main(int argc, char **argv, char
Re: [Qemu-devel] [PATCH 29/31] vl: Assert drive_new() does not fail in default_drive()
On 08.10.18 19:31, Markus Armbruster wrote: > If creating (empty) default drives fails, it's a bug. Therefore, > assert() is more appropriate than exit(1). > > Cc: Kevin Wolf > Cc: Max Reitz > Signed-off-by: Markus Armbruster > --- > vl.c | 4 +--- > 1 file changed, 1 insertion(+), 3 deletions(-) Reviewed-by: Max Reitz signature.asc Description: OpenPGP digital signature
Re: [Qemu-devel] [PATCH v2 7/7] block/qcow2-refcount: fix out-of-file L2 entries to be read-as-zero
On 17.08.18 14:22, Vladimir Sementsov-Ogievskiy wrote:
> Rewrite corrupted L2 table entry, which reference space out of
> underlying file.
>
> Make this L2 table entry read-as-all-zeros without any allocation.
>
> Signed-off-by: Vladimir Sementsov-Ogievskiy
> ---
> block/qcow2-refcount.c | 32
> 1 file changed, 32 insertions(+)
>
> diff --git a/block/qcow2-refcount.c b/block/qcow2-refcount.c
> index 3c004e5bfe..3de3768a3c 100644
> --- a/block/qcow2-refcount.c
> +++ b/block/qcow2-refcount.c
> @@ -1720,8 +1720,30 @@ static int check_refcounts_l2(BlockDriverState *bs,
> BdrvCheckResult *res,
> /* Mark cluster as used */
> csize = (((l2_entry >> s->csize_shift) & s->csize_mask) + 1) *
> BDRV_SECTOR_SIZE;
> +if (csize > s->cluster_size) {
> +ret = fix_l2_entry_to_zero(
> +bs, res, fix, l2_offset, i, active,
> +"compressed cluster larger than cluster: size 0x%"
> +PRIx64, csize);
> +if (ret < 0) {
> +goto fail;
> +}
> +continue;
> +}
> +
This seems recoverable, isn't it? Can we not try to just limit the
csize, or decompress the cluster with the given csize from the given
offset, disregarding the cluster limit?
> coffset = l2_entry & s->cluster_offset_mask &
>~(BDRV_SECTOR_SIZE - 1);
> +if (coffset >= bdrv_getlength(bs->file->bs)) {
> +ret = fix_l2_entry_to_zero(
> +bs, res, fix, l2_offset, i, active,
> +"compressed cluster out of file: offset 0x%" PRIx64,
> +coffset);
> +if (ret < 0) {
> +goto fail;
> +}
> +continue;
> +}
> +
> ret = qcow2_inc_refcounts_imrt(bs, res,
> refcount_table,
> refcount_table_size,
> coffset, csize);
> @@ -1748,6 +1770,16 @@ static int check_refcounts_l2(BlockDriverState *bs,
> BdrvCheckResult *res,
> {
> uint64_t offset = l2_entry & L2E_OFFSET_MASK;
>
> +if (offset >= bdrv_getlength(bs->file->bs)) {
> +ret = fix_l2_entry_to_zero(
> +bs, res, fix, l2_offset, i, active,
> +"cluster out of file: offset 0x%" PRIx64, offset);
> +if (ret < 0) {
> +goto fail;
> +}
> +continue;
> +}
> +
These other two look OK, but they have another issue: If this is a v2
image, you cannot create zero clusters; so you'll have to unallocate the
cluster in that case.
Max
signature.asc
Description: OpenPGP digital signature
Re: [Qemu-devel] [PATCH 30/31] blockdev: Convert drive_new() to Error
On 10/8/18 12:31 PM, Markus Armbruster wrote: Calling error_report() from within a a function that takes an Error ** s/a a/a/ argument is suspicious. drive_new() does that, and its caller drive_init_func() then exit()s. Its caller main(), via qemu_opts_foreach(), is fine with it, but clean it up anyway: * Convert drive_new() to Error * Update add_init_drive() to report the error received from drive_new(). * Make main() pass _fatal through qemu_opts_foreach(), drive_init_func() to drive_new() * Make default_drive() pass _abort through qemu_opts_foreach(), drive_init_func() to drive_new() Cc: Kevin Wolf Cc: Max Reitz Signed-off-by: Markus Armbruster --- blockdev.c| 27 ++- device-hotplug.c | 5 - include/sysemu/blockdev.h | 3 ++- vl.c | 11 --- 4 files changed, 24 insertions(+), 22 deletions(-) Reviewed-by: Eric Blake -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org
Re: [Qemu-devel] [PATCH 29/31] vl: Assert drive_new() does not fail in default_drive()
On 10/8/18 12:31 PM, Markus Armbruster wrote: If creating (empty) default drives fails, it's a bug. Therefore, assert() is more appropriate than exit(1). Cc: Kevin Wolf Cc: Max Reitz Signed-off-by: Markus Armbruster --- vl.c | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) The assert disappears in the next patch (replaced by an _fatal), but the separation of patches makes sense. Reviewed-by: Eric Blake -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org
Re: [Qemu-devel] [PATCH 28/31] fsdev: Clean up error reporting in qemu_fsdev_add()
On 10/8/18 12:31 PM, Markus Armbruster wrote: Calling error_report() from within a a function that takes an Error ** s/a a/a/ argument is suspicious. qemu_fsdev_add() does that, and its caller fsdev_init_func() then fails without setting an error. Its caller main(), via qemu_opts_foreach(), is fine with it, but clean it up anyway. -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org
Re: [Qemu-devel] [PATCH v2 2/7] block/qcow2-refcount: avoid eating RAM
On 08.10.18 22:22, Vladimir Sementsov-Ogievskiy wrote:
>
>
> On 10/08/2018 06:31 PM, Max Reitz wrote:
>> On 17.08.18 14:22, Vladimir Sementsov-Ogievskiy wrote:
>>> qcow2_inc_refcounts_imrt() (through realloc_refcount_array()) can eat
>>> an unpredictable amount of memory on corrupted table entries, which are
>>> referencing regions far beyond the end of file.
>>>
>>> Prevent this, by skipping such regions from further processing.
>>>
>>> Signed-off-by: Vladimir Sementsov-Ogievskiy
>>> ---
>>> block/qcow2-refcount.c | 14 ++
>>> 1 file changed, 14 insertions(+)
>>>
>>> diff --git a/block/qcow2-refcount.c b/block/qcow2-refcount.c
>>> index 615847eb09..566c19fbfa 100644
>>> --- a/block/qcow2-refcount.c
>>> +++ b/block/qcow2-refcount.c
>>> @@ -1499,12 +1499,26 @@ int qcow2_inc_refcounts_imrt(BlockDriverState *bs,
>>> BdrvCheckResult *res,
>>> {
>>> BDRVQcow2State *s = bs->opaque;
>>> uint64_t start, last, cluster_offset, k, refcount;
>>> +int64_t file_len;
>>> int ret;
>>>
>>> if (size <= 0) {
>>> return 0;
>>> }
>>>
>>> +file_len = bdrv_getlength(bs->file->bs);
>>> +if (file_len < 0) {
>>> +return file_len;
>>> +}
>>
>> Doesn't this slow things down? Can we not cache the length somewhere
>> and update it whenever the image is modified?
>
>
> hmm. bdrv_getlength is used everywhere in Qemu, and I don't think it is
> good idea to improve it locally for these series. If we can improve it
> somehow with a cache or something like this, it should be done for all
> users and therefore it is outside of these series..
I wanted to write: Sure it's used everywhere, but usually that is before
someone performs some I/O, so it isn't too bad. But this is a function
that's suppose to just increment a couple of values in memory, which is
different.
However, I put the "wanted to write" prefix there, because: I knew that
we already have a central cache for bdrv_getlength(), but it isn't used
when the block driver reports has_variable_length as true. I thought
file-posix did that. But it only does so for CD-ROM devices.
So I think it should be OK to call the function here, yes.
>>> +
>>> +if (offset + size - file_len > s->cluster_size) {
>>> +fprintf(stderr, "ERROR: counting reference for region exceeding
>>> the "
>>> +"end of the file by more than one cluster: offset 0x%"
>>> PRIx64
>>> +" size 0x%" PRIx64 "\n", offset, size);
>>
>> Why is one cluster OK? Is there a specific case you're trying to catch
>> here?
>
> raw file under qcow2 may be not aligned in real size to qcow2 cluster,
> as I understand, it's normal for the last cluster to be semi-allocated
Ah, that's true, thanks. I'd appreciate a comment here, though, and in
that case I think we don't need to check whether the reference is off by
more than a cluster, but whether it's off by a cluster or more (so >=).
Max
signature.asc
Description: OpenPGP digital signature
Re: [Qemu-devel] [PATCH 18/31] qom: Clean up error reporting in user_creatable_add_opts_foreach()
On 10/8/18 12:31 PM, Markus Armbruster wrote: Calling error_report() in a function that takes an Error ** argument is suspicious. user_creatable_add_opts_foreach() does that, and then fails without setting an error. Its caller main(), via qemu_opts_foreach(), is fine with it, but clean it up anyway. Cc: Daniel P. Berrangé Signed-off-by: Markus Armbruster --- qemu-io.c | 8 +++- qemu-nbd.c | 8 +++- qom/object_interfaces.c | 4 +--- vl.c| 16 ++-- 4 files changed, 13 insertions(+), 23 deletions(-) While I could take this through my NBD tree, I think it makes more sense to go through your error tree with the rest of the series. Reviewed-by: Eric Blake -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org
Re: [Qemu-devel] [PATCH 02/31] block: Use warn_report() & friends to report warnings
On 10/8/18 12:30 PM, Markus Armbruster wrote: Calling error_report() in a function that takes an Error ** argument is suspicious. Convert a few that are actually warnings to warn_report(). While there, split warnings consisting of multiple sentences to conform to conventions spelled out in warn_report()'s contract, and improve a rather useless warning in sheepdog.c. Reviewed-by: Eric Blake -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org
Re: [Qemu-devel] [PATCH 01/31] Use error_fatal to simplify obvious fatal errors (again)
On 10/8/18 12:30 PM, Markus Armbruster wrote:
Add a slight improvement of the Coccinelle semantic patch from commit
07d04a0219b,
which shares the same commit title, but does not actually have a
semantic patch, but rather defers to the even older 007b065. But I'm
not too worried about either the duplicated commit title nor the chain
of references to follow - as this is a no-semantic-change patch rather
than a bugfix, it's less likely to cause confusion to any downstream
backport efforts.
and use it to clean up. It leaves dead Error * variables
behind, cleaned up manually.
Coccinelle can handle that too, if we want to make the .cocci file
longer (but off-hand, I don't remember the exact semantic patch formula
to express a variable that is initialized but then never used, as a
result of applying earlier semantic patches). So the manual cleanup for
now still seems tractable.
Cc: David Gibson
Cc: Alexander Graf
Cc: Eric Blake
Cc: Paolo Bonzini
Signed-off-by: Markus Armbruster
---
hw/intc/xics_kvm.c | 7 +--
qemu-nbd.c | 6 +-
scripts/coccinelle/use-error_fatal.cocci | 20
vl.c | 7 +--
4 files changed, 23 insertions(+), 17 deletions(-)
create mode 100644 scripts/coccinelle/use-error_fatal.cocci
+++ b/scripts/coccinelle/use-error_fatal.cocci
@@ -0,0 +1,20 @@
+@@
+type T;
+identifier FUN, RET;
+expression list ARGS;
+expression ERR, EC, FAIL;
The slight improvement from the original git commit log script is the
addition of FAIL,
+@@
+(
+-T RET = FUN(ARGS, );
++T RET = FUN(ARGS, _fatal);
+|
+-RET = FUN(ARGS, );
++RET = FUN(ARGS, _fatal);
+|
+-FUN(ARGS, );
++FUN(ARGS, _fatal);
+)
+-if (FAIL) {
and a check for arbitrary condition FAIL rather than a more specific ERR
!= NULL. Makes sense.
+-error_report_err(ERR);
+-exit(EC);
+-}
Reviewed-by: Eric Blake
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3266
Virtualization: qemu.org | libvirt.org
Re: [Qemu-devel] [Qemu-block] [PATCH v11 07/31] iotests.py: Add node_info()
On 08.10.18 21:59, John Snow wrote:
>
>
> On 10/08/2018 03:57 PM, Max Reitz wrote:
>> On 08.10.18 21:34, John Snow wrote:
>>>
>>>
>>> On 10/05/2018 07:39 PM, Max Reitz wrote:
This function queries a node; since we cannot do that right now, it
executes query-named-block-nodes and returns the matching node's object.
Signed-off-by: Max Reitz
---
tests/qemu-iotests/iotests.py | 7 +++
1 file changed, 7 insertions(+)
diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
index 5c45788dac..604f200600 100644
--- a/tests/qemu-iotests/iotests.py
+++ b/tests/qemu-iotests/iotests.py
@@ -465,6 +465,13 @@ class VM(qtest.QEMUQtestMachine):
else:
iotests.log(ev)
+def node_info(self, node_name):
+nodes = self.qmp('query-named-block-nodes')
+for x in nodes['return']:
+if x['node-name'] == node_name:
+return x
+return None
+
index_re = re.compile(r'([^\[]+)\[([^\]]+)\]')
>>>
>>> Reviewed-by: John Snow
>>>
>>> Do we ever want to revisit the idea that our API should not do any
>>> pre-filtering and that it's always up to the client to do so?
>>
>> I mean we certainly want to revisit the idea that we should have a
>> proper query API.
>>
>> But what exactly do you mean by pre-filtering?
>>
>> Max
>>
>
> i.e. applying selection criteria before responding to the query. e.g.,
> by looking for matches on node-name.
>
> (Did I use i.e. and e.g. right?)
So you mean our API should do pre-filtering at the request of the client
and the client shouldn't be fed all nodes and then have to do
post-filtering?
Well, I certainly think we want a query command that doesn't return all
nodes. I personally would prefer something that only returns a single
node in the graph and its relationships, although I'm not sure whether
that is very efficient. Maybe something like inverse blockdev-add, were
the user can specify a depth on how deep references should be expanded.
Overall, I'm of the impression we want to have blockdev-reopen first so
that we have a good idea of the type of information we want to report
for a node; that being its node options, first and foremost. Probably
some stats as well, though.
Max
(The i.e. looks weird because you're not expanding a thought you've just
given, but you're actually responding to my question. It's like saying
"I want feature A." -- "We can do that, do you have any other remarks?"
-- "And I want B, too." So I would have started either with "e.g." (if
you think there are other things that could be meant by pre-filtering),
or by e.g. "I thought of". Probably the latter, since that is how I
interpreted your "i.e.".)
((Did I use the semicolon correctly?))
signature.asc
Description: OpenPGP digital signature
Re: [Qemu-devel] [PATCH v2 2/7] block/qcow2-refcount: avoid eating RAM
On 10/08/2018 06:31 PM, Max Reitz wrote:
> On 17.08.18 14:22, Vladimir Sementsov-Ogievskiy wrote:
>> qcow2_inc_refcounts_imrt() (through realloc_refcount_array()) can eat
>> an unpredictable amount of memory on corrupted table entries, which are
>> referencing regions far beyond the end of file.
>>
>> Prevent this, by skipping such regions from further processing.
>>
>> Signed-off-by: Vladimir Sementsov-Ogievskiy
>> ---
>> block/qcow2-refcount.c | 14 ++
>> 1 file changed, 14 insertions(+)
>>
>> diff --git a/block/qcow2-refcount.c b/block/qcow2-refcount.c
>> index 615847eb09..566c19fbfa 100644
>> --- a/block/qcow2-refcount.c
>> +++ b/block/qcow2-refcount.c
>> @@ -1499,12 +1499,26 @@ int qcow2_inc_refcounts_imrt(BlockDriverState *bs,
>> BdrvCheckResult *res,
>> {
>> BDRVQcow2State *s = bs->opaque;
>> uint64_t start, last, cluster_offset, k, refcount;
>> +int64_t file_len;
>> int ret;
>>
>> if (size <= 0) {
>> return 0;
>> }
>>
>> +file_len = bdrv_getlength(bs->file->bs);
>> +if (file_len < 0) {
>> +return file_len;
>> +}
>
> Doesn't this slow things down? Can we not cache the length somewhere
> and update it whenever the image is modified?
hmm. bdrv_getlength is used everywhere in Qemu, and I don't think it is
good idea to improve it locally for these series. If we can improve it
somehow with a cache or something like this, it should be done for all
users and therefore it is outside of these series..
>
>> +
>> +if (offset + size - file_len > s->cluster_size) {
>> +fprintf(stderr, "ERROR: counting reference for region exceeding the
>> "
>> +"end of the file by more than one cluster: offset 0x%"
>> PRIx64
>> +" size 0x%" PRIx64 "\n", offset, size);
>
> Why is one cluster OK? Is there a specific case you're trying to catch
> here?
raw file under qcow2 may be not aligned in real size to qcow2 cluster,
as I understand, it's normal for the last cluster to be semi-allocated
>
> Max
>
>> +res->corruptions++;
>> +return 0;
>> +}
>> +
>> start = start_of_cluster(s, offset);
>> last = start_of_cluster(s, offset + size - 1);
>> for(cluster_offset = start; cluster_offset <= last;
>>
>
>
Re: [Qemu-devel] [PATCH] migration: invalidate cache before source start
On 10/08/2018 11:36 AM, Vladimir Sementsov-Ogievskiy wrote:
> 26.06.2018 11:44, Vladimir Sementsov-Ogievskiy wrote:
>> 25.06.2018 20:50, Dr. David Alan Gilbert wrote:
>>> * Dr. David Alan Gilbert (dgilb...@redhat.com) wrote:
* Vladimir Sementsov-Ogievskiy (vsement...@virtuozzo.com) wrote:
> 15.06.2018 15:06, Dr. David Alan Gilbert wrote:
>> * Vladimir Sementsov-Ogievskiy (vsement...@virtuozzo.com) wrote:
>>> Invalidate cache before source start in case of failed migration.
>>>
>>> Signed-off-by: Vladimir Sementsov-Ogievskiy
>>>
>> Why doesn't the code at the bottom of migration_completion,
>> fail_invalidate: and the code in migrate_fd_cancel handle this?
>>
>> What case did you see it in that those didn't handle?
>> (Also I guess it probably should set s->block_inactive = false)
> on source I see:
>
> 81392@1529065750.766289:migrate_set_state new state 7
> 81392@1529065750.766330:migration_thread_file_err
> 81392@1529065750.766332:migration_thread_after_loop
>
> so, we are leaving loop on
> if (qemu_file_get_error(s->to_dst_file)) {
> migrate_set_state(>state, current_active_state,
> MIGRATION_STATUS_FAILED);
> trace_migration_thread_file_err();
> break;
> }
>
> and skip migration_completion()
>>
>>
>> John is right, this ls an unrelated log, here we fail before
>> inactivation and there are no problems.
>>
>> Actual problem is when we fail in postcopy_start, at the end. And
>> source log looks like:
>>
>> 84297@1530001796.287344:migrate_set_state new state 1
>> 84297@1530001796.287374:migration_fd_outgoing fd=101
>> 84297@1530001796.287383:migration_set_outgoing_channel
>> ioc=0x56363454d630 ioctype=qio-channel-socket hostname=(null)
>> 84297@1530001796.294032:migration_bitmap_sync_start
>> 84297@1530001796.300483:migration_bitmap_sync_end dirty_pages 932
>> 84297@1530001796.300561:migrate_set_state new state 4
>> 84297@1530001796.300588:migration_thread_setup_complete
>> 84297@1530001796.300593:migrate_pending pending size 1107976192 max 0
>> (pre = 0 compat=1107976192 post=0)
>> 84297@1530001796.300595:migrate_set_state new state 5
>> Tap fd 33 disable, ret 0
>> 84297@1530001796.426477:migration_bitmap_sync_start
>> 84297@1530001796.433227:migration_bitmap_sync_end dirty_pages 1091
>> 84297@1530001796.439077:migrate_global_state_pre_save saved state:
>> running
>> 2018-06-26T08:29:56.439134Z qemu-kvm: postcopy_start: Migration stream
>> errored -5
>> 84297@1530001796.439141:migrate_set_state new state 7
>> 84297@1530001796.439181:migration_thread_after_loop
>> Tap fd 33 enable, ret 0
>> 84297@1530001796.453639:migrate_fd_cleanup
>> qemu-kvm: block/io.c:1655: bdrv_co_pwritev: Assertion
>> `!(bs->open_flags & 0x0800)' failed.
>> 2018-06-26 08:29:56.605+: shutting down, reason=crashed
>>
>>
Yeh, OK; I'd seen soemthing else a few days ago, where a cancellation
test that had previously ended with a 'cancelled' state has now
ended up
in 'failed' (which is the state 7 you have above).
I suspect there's something else going on as well; I think what is
supposed to happen in the case of 'cancel' is that it spins in
'cancelling' for
a while in migrate_fd_cancel and then at the bottom of
migrate_fd_cancel
it does the recovery, but because it's going to failed instead, then
it's jumping over that recovery.
>>> Going back and actually looking at the patch again;
>>> can I ask for 1 small change;
>>> Can you set s->block_inactive = false in the case where you
>>> don't get the local_err (Like we do at the bottom of migrate_fd_cancel)
>>>
>>>
>>> Does that make sense?
>>
>> Ok, I'll resend.
>>
>> Hm, looks like I'm fixing an outdated version (based on v2.9.0) And my
>> reproduce isn't appropriate for upstream.
>> But looks like current code have a possibility of the same fail:
>>
>> postcopy_start()
>>
>> ret = qemu_file_get_error(ms->to_dst_file);
>> if (ret) {
>> error_report("postcopy_start: Migration stream errored");
>>
>> leads to "return MIG_ITERATE_SKIP;" in migration_iteration_run
>>
>> then the loop should finish, as state should be
>> MIGRATION_STATUS_FAILED, so we will not call migration_completion.
>>
>> Hm, I have questions now:
>>
>> 1. should we check s->block_inactive, and if it is false, don't
>> invalidate? it is done in migrate_fd_cancel(), but not don in
>> migration_completion().
>> 2. should we call qemu_mutex_lock_iothread() like in
>> migration_completion()? Why is it needed in migration_completion(),
>> when vm is not running?
>
>
> Hm, forgotten thread, I should resend, but what do you think about these
> questions?
>
Personally, I can't remember where we stand on this thread at all. If
you have changes that you think are still important (because you ran
into the issue again), probably best to send a respin (with a new cover
letter and
Re: [Qemu-devel] [PATCH v2 2/7] block/qcow2-refcount: avoid eating RAM
On 10/08/2018 06:31 PM, Max Reitz wrote:
> On 17.08.18 14:22, Vladimir Sementsov-Ogievskiy wrote:
>> qcow2_inc_refcounts_imrt() (through realloc_refcount_array()) can eat
>> an unpredictable amount of memory on corrupted table entries, which are
>> referencing regions far beyond the end of file.
>>
>> Prevent this, by skipping such regions from further processing.
>>
>> Signed-off-by: Vladimir Sementsov-Ogievskiy
>> ---
>> block/qcow2-refcount.c | 14 ++
>> 1 file changed, 14 insertions(+)
>>
>> diff --git a/block/qcow2-refcount.c b/block/qcow2-refcount.c
>> index 615847eb09..566c19fbfa 100644
>> --- a/block/qcow2-refcount.c
>> +++ b/block/qcow2-refcount.c
>> @@ -1499,12 +1499,26 @@ int qcow2_inc_refcounts_imrt(BlockDriverState *bs,
>> BdrvCheckResult *res,
>> {
>> BDRVQcow2State *s = bs->opaque;
>> uint64_t start, last, cluster_offset, k, refcount;
>> +int64_t file_len;
>> int ret;
>>
>> if (size <= 0) {
>> return 0;
>> }
>>
>> +file_len = bdrv_getlength(bs->file->bs);
>> +if (file_len < 0) {
>> +return file_len;
>> +}
>
> Doesn't this slow things down? Can we not cache the length somewhere
> and update it whenever the image is modified?
hmm. bdrv_getlength is used everywhere in Qemu, and I don't think it is
good idea to improve it locally for these series. If we can improve it
somehow with a cache or something like this, it should be done for all
users and therefore it is outside of these series..
>
>> +
>> +if (offset + size - file_len > s->cluster_size) {
>> +fprintf(stderr, "ERROR: counting reference for region exceeding the
>> "
>> +"end of the file by more than one cluster: offset 0x%"
>> PRIx64
>> +" size 0x%" PRIx64 "\n", offset, size);
>
> Why is one cluster OK? Is there a specific case you're trying to catch
> here?
raw file under qcow2 may be not aligned in real size to qcow2 cluster,
as I understand, it's normal for the last cluster to be semi-allocated
>
> Max
>
>> +res->corruptions++;
>> +return 0;
>> +}
>> +
>> start = start_of_cluster(s, offset);
>> last = start_of_cluster(s, offset + size - 1);
>> for(cluster_offset = start; cluster_offset <= last;
>>
>
>
Re: [Qemu-devel] [RFC 2/6] cputlb: do not evict invalid entries to the vtlb
On Mon, Oct 08, 2018 at 12:46:26 -0700, Richard Henderson wrote:
> On 10/8/18 7:42 AM, Emilio G. Cota wrote:
> > On Sun, Oct 07, 2018 at 19:09:01 -0700, Richard Henderson wrote:
> >> On 10/6/18 2:45 PM, Emilio G. Cota wrote:
> >>> Currently we evict an entry to the victim TLB when it doesn't match
> >>> the current address. But it could be that there's no match because
> >>> the current entry is invalid. Do not evict the entry to the vtlb
> >>> in that case.
> >>>
> >>> This change will help us keep track of the TLB's use rate.
> >>>
> >>> Signed-off-by: Emilio G. Cota
> >>> ---
> >>> include/exec/cpu-all.h | 14 ++
> >>> accel/tcg/cputlb.c | 2 +-
> >>> 2 files changed, 15 insertions(+), 1 deletion(-)
> >>>
> >>> diff --git a/include/exec/cpu-all.h b/include/exec/cpu-all.h
> >>> index 117d2fbbca..d938dedafc 100644
> >>> --- a/include/exec/cpu-all.h
> >>> +++ b/include/exec/cpu-all.h
> >>> @@ -362,6 +362,20 @@ static inline bool tlb_hit(target_ulong tlb_addr,
> >>> target_ulong addr)
> >>> return tlb_hit_page(tlb_addr, addr & TARGET_PAGE_MASK);
> >>> }
> >>>
> >>> +/**
> >>> + * tlb_is_valid - return true if at least one of the addresses is valid
> >>> + * @te: pointer to CPUTLBEntry
> >>> + *
> >>> + * This is useful when we don't have a particular address to compare
> >>> against,
> >>> + * and we just want to know whether any entry holds valid data.
> >>> + */
> >>> +static inline bool tlb_is_valid(const CPUTLBEntry *te)
> >>> +{
> >>> +return !(te->addr_read & TLB_INVALID_MASK) ||
> >>> + !(te->addr_write & TLB_INVALID_MASK) ||
> >>> + !(te->addr_code & TLB_INVALID_MASK);
> >>> +}
> >>
> >> No, I think you misunderstand.
> >>
> >> First, TLB_INVALID_MASK is only ever set for addr_write, in response to
> >> PAGE_WRITE_INV. Second, an entry that is invalid for write is still valid
> >> for
> >> read+exec. So there is benefit to swapping it out to the victim cache.
> >>
> >> This is used by the s390x target to make the "lowpage" read-only, which is
> >> a
> >> special architected 512 byte range within pages 0 and 1. This is done by
> >> forcing writes, but not reads, back through tlb_fill.
> >
> > Aah I see. The point is to avoid pushing to the victim cache
> > an entry that is all invalid, not just partially invalid.
>
> I've slightly misspoken here. It is (ab)used for the s390 thing, but that bit
> is also set by memset -1. Perhaps you might check
>
> static inline bool tlb_is_invalid(const CPUTLBEntry *te)
> {
> return te->addr_read & te->addr_write & te->addr_code & TLB_INVALID_MASK;
> }
>
> That is, all forms of access have the INVALID bit set.
I see. I just ran a few tests and the below gives the best
performance, so I'll go with it:
static inline bool tlb_is_empty(const CPUTLBEntry *te)
{
return te->addr_read == -1 && te->addr_write == -1 && te->addr_code == -1;
}
I renamed it from "invalid" to "empty" to avoid even thinking about
the invalid flag.
Thanks,
Emilio
Re: [Qemu-devel] [PATCH v2 1/2] MAINTAINERS: Replace myself with John Snow for block jobs
On 09/26/2018 02:05 PM, Jeff Cody wrote: > I'll not be involved with day-to-day qemu development, and John > Snow is a block jobs wizard. Have him take over block job > maintainership duties. > ... I didn't realize he wrote it like this, and I am told I should accept compliments, ... Well, factually it's Kevin and I who have worked on them the most lately, and Kevin's already got enough stuff, so I think it's okay for me to take this unless someone disagrees. > Signed-off-by: Jeff Cody > --- > MAINTAINERS | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/MAINTAINERS b/MAINTAINERS > index ce7c351afa..0e22b795e6 100644 > --- a/MAINTAINERS > +++ b/MAINTAINERS > @@ -1455,7 +1455,7 @@ F: include/scsi/* > F: scsi/* > > Block Jobs > -M: Jeff Cody > +M: John Snow Acked-by: John Snow `*~ pour one out for your homies ~*' > L: qemu-bl...@nongnu.org > S: Supported > F: blockjob.c > @@ -1468,7 +1468,7 @@ F: block/commit.c > F: block/stream.c > F: block/mirror.c > F: qapi/job.json > -T: git git://github.com/codyprime/qemu-kvm-jtc.git block > +T: git git://github.com/jnsnow/qemu.git jobs > > Block QAPI, monitor, command line > M: Markus Armbruster >
Re: [Qemu-devel] [PATCH v2 6/7] block/qcow2-refcount: fix out-of-file L1 entries to be zero
On 17.08.18 14:22, Vladimir Sementsov-Ogievskiy wrote: > Zero out corrupted L1 table entry, which reference L2 table out of > underlying file. > Zero L1 table entry means that "the L2 table and all clusters described > by this L2 table are unallocated." > > Signed-off-by: Vladimir Sementsov-Ogievskiy > --- > block/qcow2-refcount.c | 44 > 1 file changed, 44 insertions(+) Hm. The specification actually says nothing about offsets being allowed past the end of the file, and I don't think we ever use them (outside of a very short period during image creation, where we point to refcount structures beyond the EOF). So the patch looks OK to me, although I'd still prefer a separate fprintf() and think it would be fine for check_refcounts_l1() to call fix_table_entry() directly. Max signature.asc Description: OpenPGP digital signature
[Qemu-devel] [Bug 1796754] [NEW] ioctl SIOCGIFCONF causes qemu-aarch64-static to crash with "received signal outside vCPU context"
Public bug reported: To reproduce it, compile the attached crash.c under aarch64 to a.out and execute on x86_64 qemu-aarch64-static ./a.out It will print the following and crash: socket=3 qemu:handle_cpu_signal received signal outside vCPU context @ pc=0x60038cd6 qemu:handle_cpu_signal received signal outside vCPU context @ pc=0x6000157a The version of qemu-aarch64-static is qemu-aarch64 version 3.0.0 (qemu-3.0.0-1.fc29) Copyright (c) 2003-2017 Fabrice Bellard and the QEMU Project developers But it did also happen in previous versions so it is not a regression but a bug existed ever since. ** Affects: qemu Importance: Undecided Status: New ** Tags: aarch64 arm linux-user qemu ** Attachment added: "crash.c" https://bugs.launchpad.net/bugs/1796754/+attachment/5198861/+files/crash.c ** Tags added: aarch64 ** Tags added: linux-user qemu ** Tags added: arm -- You received this bug notification because you are a member of qemu- devel-ml, which is subscribed to QEMU. https://bugs.launchpad.net/bugs/1796754 Title: ioctl SIOCGIFCONF causes qemu-aarch64-static to crash with "received signal outside vCPU context" Status in QEMU: New Bug description: To reproduce it, compile the attached crash.c under aarch64 to a.out and execute on x86_64 qemu-aarch64-static ./a.out It will print the following and crash: socket=3 qemu:handle_cpu_signal received signal outside vCPU context @ pc=0x60038cd6 qemu:handle_cpu_signal received signal outside vCPU context @ pc=0x6000157a The version of qemu-aarch64-static is qemu-aarch64 version 3.0.0 (qemu-3.0.0-1.fc29) Copyright (c) 2003-2017 Fabrice Bellard and the QEMU Project developers But it did also happen in previous versions so it is not a regression but a bug existed ever since. To manage notifications about this bug go to: https://bugs.launchpad.net/qemu/+bug/1796754/+subscriptions
Re: [Qemu-devel] [Qemu-block] [PATCH v11 07/31] iotests.py: Add node_info()
On 08.10.18 21:34, John Snow wrote:
>
>
> On 10/05/2018 07:39 PM, Max Reitz wrote:
>> This function queries a node; since we cannot do that right now, it
>> executes query-named-block-nodes and returns the matching node's object.
>>
>> Signed-off-by: Max Reitz
>> ---
>> tests/qemu-iotests/iotests.py | 7 +++
>> 1 file changed, 7 insertions(+)
>>
>> diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
>> index 5c45788dac..604f200600 100644
>> --- a/tests/qemu-iotests/iotests.py
>> +++ b/tests/qemu-iotests/iotests.py
>> @@ -465,6 +465,13 @@ class VM(qtest.QEMUQtestMachine):
>> else:
>> iotests.log(ev)
>>
>> +def node_info(self, node_name):
>> +nodes = self.qmp('query-named-block-nodes')
>> +for x in nodes['return']:
>> +if x['node-name'] == node_name:
>> +return x
>> +return None
>> +
>>
>> index_re = re.compile(r'([^\[]+)\[([^\]]+)\]')
>>
>>
>
> Reviewed-by: John Snow
>
> Do we ever want to revisit the idea that our API should not do any
> pre-filtering and that it's always up to the client to do so?
I mean we certainly want to revisit the idea that we should have a
proper query API.
But what exactly do you mean by pre-filtering?
Max
signature.asc
Description: OpenPGP digital signature
Re: [Qemu-devel] [Qemu-block] [PATCH v11 07/31] iotests.py: Add node_info()
On 10/08/2018 03:57 PM, Max Reitz wrote:
> On 08.10.18 21:34, John Snow wrote:
>>
>>
>> On 10/05/2018 07:39 PM, Max Reitz wrote:
>>> This function queries a node; since we cannot do that right now, it
>>> executes query-named-block-nodes and returns the matching node's object.
>>>
>>> Signed-off-by: Max Reitz
>>> ---
>>> tests/qemu-iotests/iotests.py | 7 +++
>>> 1 file changed, 7 insertions(+)
>>>
>>> diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
>>> index 5c45788dac..604f200600 100644
>>> --- a/tests/qemu-iotests/iotests.py
>>> +++ b/tests/qemu-iotests/iotests.py
>>> @@ -465,6 +465,13 @@ class VM(qtest.QEMUQtestMachine):
>>> else:
>>> iotests.log(ev)
>>>
>>> +def node_info(self, node_name):
>>> +nodes = self.qmp('query-named-block-nodes')
>>> +for x in nodes['return']:
>>> +if x['node-name'] == node_name:
>>> +return x
>>> +return None
>>> +
>>>
>>> index_re = re.compile(r'([^\[]+)\[([^\]]+)\]')
>>>
>>>
>>
>> Reviewed-by: John Snow
>>
>> Do we ever want to revisit the idea that our API should not do any
>> pre-filtering and that it's always up to the client to do so?
>
> I mean we certainly want to revisit the idea that we should have a
> proper query API.
>
> But what exactly do you mean by pre-filtering?
>
> Max
>
i.e. applying selection criteria before responding to the query. e.g.,
by looking for matches on node-name.
(Did I use i.e. and e.g. right?)
--js
Re: [Qemu-devel] [PATCH v1 06/12] net: cadence_gem: Add support for selecting the DMA MemoryRegion
On Mon, Oct 08, 2018 at 01:30:20PM +0100, Peter Maydell wrote:
> On 3 October 2018 at 16:07, Edgar E. Iglesias
> wrote:
> > From: "Edgar E. Iglesias"
> >
> > Add support for selecting the Memory Region that the GEM
> > will do DMA to.
> >
> > Signed-off-by: Edgar E. Iglesias
> > ---
> > hw/net/cadence_gem.c | 63
> >
> > include/hw/net/cadence_gem.h | 2 ++
> > 2 files changed, 43 insertions(+), 22 deletions(-)
> >
> > diff --git a/hw/net/cadence_gem.c b/hw/net/cadence_gem.c
> > index 759c1d7..ab02515 100644
> > --- a/hw/net/cadence_gem.c
> > +++ b/hw/net/cadence_gem.c
> > @@ -28,6 +28,7 @@
> > #include "hw/net/cadence_gem.h"
> > #include "qapi/error.h"
> > #include "qemu/log.h"
> > +#include "sysemu/dma.h"
> > #include "net/checksum.h"
> >
> > #ifdef CADENCE_GEM_ERR_DEBUG
> > @@ -835,9 +836,9 @@ static void gem_get_rx_desc(CadenceGEMState *s, int q)
> > {
> > DB_PRINT("read descriptor 0x%x\n", (unsigned)s->rx_desc_addr[q]);
> > /* read current descriptor */
> > -cpu_physical_memory_read(s->rx_desc_addr[q],
> > - (uint8_t *)s->rx_desc[q],
> > - sizeof(uint32_t) * gem_get_desc_len(s, true));
> > +address_space_read(s->dma_as, s->rx_desc_addr[q],
> > MEMTXATTRS_UNSPECIFIED,
> > + (uint8_t *)s->rx_desc[q],
> > + sizeof(uint32_t) * gem_get_desc_len(s, true));
>
> At some point you might want to add support for handling "descriptor
> read/write failed", incidentally: address_space_read/write return a
> MemTxResult which you can check for != MEMTX_OK.
Yes, the GEM can report those errors to SW so that's a nice feature to follow
up with.
Thanks,
Edgar
Re: [Qemu-devel] [PATCH v1 09/12] target-arm: powerctl: Enable HVC when starting CPUs to EL2
On Mon, Oct 08, 2018 at 01:41:36PM +0100, Peter Maydell wrote:
> On 3 October 2018 at 16:07, Edgar E. Iglesias
> wrote:
> > From: "Edgar E. Iglesias"
> >
> > When QEMU provides the equivalent of the EL3 firmware, we
> > need to enable HVCs in scr_el3 when turning on CPUs that
> > target EL2.
> >
> > Signed-off-by: Edgar E. Iglesias
> > ---
> > target/arm/arm-powerctl.c | 11 +++
> > 1 file changed, 11 insertions(+)
> >
> > diff --git a/target/arm/arm-powerctl.c b/target/arm/arm-powerctl.c
> > index ce55eeb..54f2974 100644
> > --- a/target/arm/arm-powerctl.c
> > +++ b/target/arm/arm-powerctl.c
> > @@ -63,6 +63,7 @@ static void arm_set_cpu_on_async_work(CPUState
> > *target_cpu_state,
> > struct CpuOnInfo *info = (struct CpuOnInfo *) data.host_ptr;
> >
> > /* Initialize the cpu we are turning on */
> > +qemu_log("CPU%d reset\n", target_cpu_state->cpu_index);
>
> qemu_log logging should always be masked to a particular
> log kind (either via qemu_log_mask() or by explicit check on
> the log level). If this was accidentally left-in debug, you could
> just drop it :-)
Hah, yes this is an accidental leftover...
>
> > cpu_reset(target_cpu_state);
> > target_cpu_state->halted = 0;
> >
> > @@ -103,6 +104,16 @@ static void arm_set_cpu_on_async_work(CPUState
> > *target_cpu_state,
> > } else {
> > /* Processor is not in secure mode */
> > target_cpu->env.cp15.scr_el3 |= SCR_NS;
> > +
> > +/*
> > + * If QEMU is providing the equivalent of EL3 firmware, then we
> > need
> > + * to make sure a CPU targeting EL2 comes out of reset with a
> > + * functional HVC insn.
> > + */
> > +if (arm_feature(_cpu->env, ARM_FEATURE_EL3)
> > +&& info->target_el == 2) {
> > +target_cpu->env.cp15.scr_el3 |= SCR_HCE;
> > +}
> > }
>
> Otherwise
> Reviewed-by: Peter Maydell
> This is definitely required for PSCI and I think it makes sense
> in other cases that end up in this powerctl code with EL3 set.
Thanks,
Edgar
Re: [Qemu-devel] [PATCH v1 06/12] net: cadence_gem: Add support for selecting the DMA MemoryRegion
On Mon, Oct 08, 2018 at 01:24:51PM +0100, Peter Maydell wrote:
> On 3 October 2018 at 16:07, Edgar E. Iglesias
> wrote:
> > From: "Edgar E. Iglesias"
> >
> > Add support for selecting the Memory Region that the GEM
> > will do DMA to.
> >
> > Signed-off-by: Edgar E. Iglesias
> > ---
>
>
> > @@ -1500,6 +1506,13 @@ static void gem_realize(DeviceState *dev, Error
> > **errp)
> > CadenceGEMState *s = CADENCE_GEM(dev);
> > int i;
> >
> > +if (s->dma_mr) {
> > +s->dma_as = g_malloc0(sizeof(AddressSpace));
> > +address_space_init(s->dma_as, s->dma_mr, NULL);
>
> Why not just have the CadenceGEMState embed the AddressSpace
>
> AddressSpace dma_as;
>
> rather than doing a separate memory allocation here?
No reason not to, I copied this from a pattern in our code and didn't reflect
too much about the allocation.
I'll change it for next version.
Cheers,
Edgar
>
> > +} else {
> > +s->dma_as = _space_memory;
> > +}
>
> thanks
> -- PMM
Re: [Qemu-devel] [PATCH v2 5/7] block/qcow2-refcount: check_refcounts_l2: split fix_l2_entry_to_zero
On 17.08.18 14:22, Vladimir Sementsov-Ogievskiy wrote:
> Split entry repairing to separate function, to be reused later.
>
> Note: entry in in-memory l2 table (local variable in
> check_refcounts_l2) is not updated after this patch.
>
> Signed-off-by: Vladimir Sementsov-Ogievskiy
> ---
> block/qcow2-refcount.c | 147
> -
> 1 file changed, 109 insertions(+), 38 deletions(-)
>
> diff --git a/block/qcow2-refcount.c b/block/qcow2-refcount.c
> index 3b057b635d..d9c8cd666b 100644
> --- a/block/qcow2-refcount.c
> +++ b/block/qcow2-refcount.c
> @@ -1554,6 +1554,99 @@ enum {
> CHECK_FRAG_INFO = 0x2, /* update BlockFragInfo counters */
> };
>
> +/* Update entry in L1 or L2 table
> + *
> + * Returns: -errno if overlap check failed
> + * 0 if write failed
> + * 1 on success
> + */
> +static int write_table_entry(BlockDriverState *bs, const char *table_name,
> + uint64_t table_offset, int entry_index,
> + uint64_t new_val, int ign)
> +{
> +int ret;
> +uint64_t entry_offset =
> +table_offset + (uint64_t)entry_index * sizeof(new_val);
> +
> +cpu_to_be64s(_val);
> +ret = qcow2_pre_write_overlap_check(bs, ign, entry_offset,
> sizeof(new_val));
> +if (ret < 0) {
> +fprintf(stderr,
> +"ERROR: Can't write %s table entry: overlap check failed:
> %s\n",
I recently complained to Berto that I don't like elisions ("can't") in
user interfaces, so I suppose I'll have to complain here, too, that I'd
prefer a "Cannot".
> +table_name, strerror(-ret));
> +return ret;
> +}
> +
> +ret = bdrv_pwrite_sync(bs->file, entry_offset, _val,
> sizeof(new_val));
> +if (ret < 0) {
> +fprintf(stderr, "ERROR: Failed to overwrite %s table entry: %s\n",
> +table_name, strerror(-ret));
> +return 0;
> +}
> +
> +return 1;
> +}
> +
> +/* Try to fix (if allowed) entry in L1 or L2 table. Update @res
> correspondingly.
> + *
> + * Returns: -errno if overlap check failed
> + * 0 if entry was not updated for other reason
> + *(fixing disabled or write failed)
> + * 1 on success
> + */
> +static int fix_table_entry(BlockDriverState *bs, BdrvCheckResult *res,
> + BdrvCheckMode fix, const char *table_name,
> + uint64_t table_offset, int entry_index,
> + uint64_t new_val, int ign,
> + const char *fmt, va_list args)
> +{
> +int ret;
> +
> +fprintf(stderr, fix & BDRV_FIX_ERRORS ? "Repairing: " : "ERROR: ");
> +vfprintf(stderr, fmt, args);
> +fprintf(stderr, "\n");
If you're just going to print this here, right at the start, I think it
would be better to just do it in the caller.
Sure, with this solution the caller safes an fprintf() call, but I find
it a bit over the top to start with vararg handling here when the caller
can just do an additional fprintf().
(Also, I actually find it clearer if you have to call two separate
functions.)
> +
> +if (!(fix & BDRV_FIX_ERRORS)) {
> +res->corruptions++;
> +return 0;
> +}
> +
> +ret = write_table_entry(bs, table_name, table_offset, entry_index,
> new_val,
> +ign);
> +
> +if (ret == 1) {
> +res->corruptions_fixed++;
> +} else {
> +res->check_errors++;
> +}
> +
> +return ret;
> +}
> +
> +/* Make L2 entry to be QCOW2_CLUSTER_ZERO_PLAIN
> + *
> + * Returns: -errno if overlap check failed
> + * 0 if write failed
> + * 1 on success
> + */
> +static int fix_l2_entry_to_zero(BlockDriverState *bs, BdrvCheckResult *res,
> +BdrvCheckMode fix, int64_t l2_offset,
> +int l2_index, bool active,
> +const char *fmt, ...)
> +{
> +int ret;
> +int ign = active ? QCOW2_OL_ACTIVE_L2 : QCOW2_OL_INACTIVE_L2;
> +uint64_t l2_entry = QCOW_OFLAG_ZERO;
> +va_list args;
> +
> +va_start(args, fmt);
> +ret = fix_table_entry(bs, res, fix, "L2", l2_offset, l2_index, l2_entry,
> + ign, fmt, args);
> +va_end(args);
> +
> +return ret;
> +}
If you drop the fprintf() from fix_table_entry(), this function will
make less sense as well. Just calling fix_table_entry() directly will
be just as easy.
(Yes, I see that you use the function in patch 7 again, and yes, you'd
have to use a full line for the "active ?:" ternary, but still.)
Max
signature.asc
Description: OpenPGP digital signature
Re: [Qemu-devel] [PATCH] tests: Disable test-bdrv-drain
On 10/8/18 11:40 AM, Kevin Wolf wrote: Am 08.10.2018 um 17:43 hat Peter Maydell geschrieben: Looking at the backtraces I'm wondering if this is the result of an implicit reliance on the order in which per-thread destructors are called (which is left unspecified by POSIX) -- the destructor function qemu_thread_atexit_run() is called after some other destructor, but accesses its memory. Specifically, the memory it's trying to read looks like the __thread local variable pollfds_cleanup_notifier in util/aio-posix.c. So I think what is happening is: * util/aio-posix.c calls qemu_thread_atexit_add(), passing it a pointer to a thread-local variable pollfds_cleanup_notifier * qemu_thread_atexit_add() works by arranging to run the notifiers when its 'exit_key' variable's destructor is called * the destructor for pollfds_cleanup_notifier runs before that for exit_key, and so the qemu_thread_atexit_run() function ends up touching freed memory I'm pretty confident this analysis of the problem is correct: unfortunately I have no idea what the right way to fix it is... Yes, I agree with your analysis. If __thread variables can be destructed before pthread_key_create() destructors are called (and in particular if the former are implemented in terms of the latter), this implies at least two rules: 1. The Notfier itself can't be a TLS variable 2. The notifier callback can't access any TLS variables Of course, with these restrictions, qemu_thread_atexit_*() with its existing API is as useless as it could be. The best I can think of at the moment would be to use a separate pthread_key_create() (and therefore a separate destructor) for registering each TLS variable, so that the destructor always gets a valid pointer. Maybe move all __thread variables of a file into a single malloced struct to make it more managable (we could then keep a __thread pointer to it for convenience, but only free the struct with the pointer passed by the pthread_key destructor so that we don't have to access __thread variables in the destructor). pthread_key_create() says that a when a destructor is triggered, it sets the value of the key to NULL; but that you can once again set the key back to a non-NULL value, and that the implementation will loop at least PTHREAD_DESTRUCTOR_ITERATIONS over all destructors or until it has convinced the destructors to leave values at NULL. Thus, while you cannot guarantee ordering between destructors within a single iteration of the cleanup loop, you CAN do some sort of witness locking or down-counter where a destructor purposefully calls pthread_setspecific() to revive the value to survive into the next iteration of destructor calls, for variables which are known to be referenced by other destructors while the witness count is still high enough, as a way of imposing order between loops. -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org
Re: [Qemu-devel] [Qemu-block] [PATCH 05/10] docs/devel/testing.rst: add missing newlines after code block
On 10/04/2018 12:18 PM, Cleber Rosa wrote: > The line immediate following a ".. code::" block is considered > to contains arguments to the "code directive". The lack of a > new line gives me during at parse time: > >testing.rst:63: (ERROR/3) Error in "code" directive: >maximum 1 argument(s) allowed, 3 supplied. > >.. code:: > make check-unit V=1 > >testing.rst:120: (ERROR/3) Error in "code" directive: >maximum 1 argument(s) allowed, 3 supplied. > >.. code:: > make check-qtest V=1 > pandoc doesn't complain, but rst.ninjs.org does. What tool did you use to find these? we should formalize a formatting checker for rst files. Actually, we should formalize building our RST docs at all... --js > Let's add the missing newlines, both for consistency and to > avoid the parsing errors. > > Signed-off-by: Cleber Rosa Reviewed-by: John Snow
Re: [Qemu-devel] [RFC 2/6] cputlb: do not evict invalid entries to the vtlb
On 10/8/18 7:42 AM, Emilio G. Cota wrote:
> On Sun, Oct 07, 2018 at 19:09:01 -0700, Richard Henderson wrote:
>> On 10/6/18 2:45 PM, Emilio G. Cota wrote:
>>> Currently we evict an entry to the victim TLB when it doesn't match
>>> the current address. But it could be that there's no match because
>>> the current entry is invalid. Do not evict the entry to the vtlb
>>> in that case.
>>>
>>> This change will help us keep track of the TLB's use rate.
>>>
>>> Signed-off-by: Emilio G. Cota
>>> ---
>>> include/exec/cpu-all.h | 14 ++
>>> accel/tcg/cputlb.c | 2 +-
>>> 2 files changed, 15 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/include/exec/cpu-all.h b/include/exec/cpu-all.h
>>> index 117d2fbbca..d938dedafc 100644
>>> --- a/include/exec/cpu-all.h
>>> +++ b/include/exec/cpu-all.h
>>> @@ -362,6 +362,20 @@ static inline bool tlb_hit(target_ulong tlb_addr,
>>> target_ulong addr)
>>> return tlb_hit_page(tlb_addr, addr & TARGET_PAGE_MASK);
>>> }
>>>
>>> +/**
>>> + * tlb_is_valid - return true if at least one of the addresses is valid
>>> + * @te: pointer to CPUTLBEntry
>>> + *
>>> + * This is useful when we don't have a particular address to compare
>>> against,
>>> + * and we just want to know whether any entry holds valid data.
>>> + */
>>> +static inline bool tlb_is_valid(const CPUTLBEntry *te)
>>> +{
>>> +return !(te->addr_read & TLB_INVALID_MASK) ||
>>> + !(te->addr_write & TLB_INVALID_MASK) ||
>>> + !(te->addr_code & TLB_INVALID_MASK);
>>> +}
>>
>> No, I think you misunderstand.
>>
>> First, TLB_INVALID_MASK is only ever set for addr_write, in response to
>> PAGE_WRITE_INV. Second, an entry that is invalid for write is still valid
>> for
>> read+exec. So there is benefit to swapping it out to the victim cache.
>>
>> This is used by the s390x target to make the "lowpage" read-only, which is a
>> special architected 512 byte range within pages 0 and 1. This is done by
>> forcing writes, but not reads, back through tlb_fill.
>
> Aah I see. The point is to avoid pushing to the victim cache
> an entry that is all invalid, not just partially invalid.
I've slightly misspoken here. It is (ab)used for the s390 thing, but that bit
is also set by memset -1. Perhaps you might check
static inline bool tlb_is_invalid(const CPUTLBEntry *te)
{
return te->addr_read & te->addr_write & te->addr_code & TLB_INVALID_MASK;
}
That is, all forms of access have the INVALID bit set.
r~
Re: [Qemu-devel] [Qemu-block] [PATCH 09/10] scripts/qemu.py: use a more consistent docstring style
On 10/04/2018 12:18 PM, Cleber Rosa wrote:
> Signed-off-by: Cleber Rosa
> ---
> dtc | 2 +-
> scripts/qemu.py | 65 +++--
> 2 files changed, 42 insertions(+), 25 deletions(-)
>
> diff --git a/dtc b/dtc
> index 88f18909db..e54388015a 16
> --- a/dtc
> +++ b/dtc
> @@ -1 +1 @@
> -Subproject commit 88f18909db731a627456f26d779445f84e449536
> +Subproject commit e54388015af1fb4bf04d0bca99caba1074d9cc42
> diff --git a/scripts/qemu.py b/scripts/qemu.py
> index f099ce7278..7abe26de69 100644
> --- a/scripts/qemu.py
> +++ b/scripts/qemu.py
> @@ -53,9 +53,9 @@ class QEMUMachineAddDeviceError(QEMUMachineError):
> """
>
> class MonitorResponseError(qmp.qmp.QMPError):
> -'''
> +"""
> Represents erroneous QMP monitor reply
> -'''
> +"""
This seems obviously correct, as per the Python Dogma Handbook ...
> def __init__(self, reply):
> try:
> desc = reply["error"]["desc"]
> @@ -66,14 +66,15 @@ class MonitorResponseError(qmp.qmp.QMPError):
>
>
> class QEMUMachine(object):
> -'''A QEMU VM
> +"""
> +A QEMU VM
>
> Use this object as a context manager to ensure the QEMU process
> terminates::
>
> with VM(binary) as vm:
> ...
> # vm is guaranteed to be shut down here
> -'''
> +"""
As does this,,
>
> def __init__(self, binary, args=None, wrapper=None, name=None,
> test_dir="/var/tmp", monitor_address=None,
> @@ -135,7 +136,9 @@ class QEMUMachine(object):
> self._args.append(args)
>
> def add_fd(self, fd, fdset, opaque, opts=''):
> -'''Pass a file descriptor to the VM'''
> +"""
> +Pass a file descriptor to the VM
> +"""
However, is it established practice among ne'er-do-wells to format
one-line docstrings as three-liners? (And without punctuation to boot --
for shame!)
PEP257 suggests that one-liners are allowed, but doesn't seem to
necessitate their usage. Does this kind of change have any kind of benefit?
> options = ['fd=%d' % fd,
> 'set=%d' % fdset,
> 'opaque=%s' % opaque]
> @@ -168,7 +171,9 @@ class QEMUMachine(object):
>
> @staticmethod
> def _remove_if_exists(path):
> -'''Remove file object at path if it exists'''
> +"""
> +Remove file object at path if it exists
> +"""
> try:
> os.remove(path)
> except OSError as exception:
> @@ -271,7 +276,9 @@ class QEMUMachine(object):
> raise
>
> def _launch(self):
> -'''Launch the VM and establish a QMP connection'''
> +"""
> +Launch the VM and establish a QMP connection
> +"""
> devnull = open(os.path.devnull, 'rb')
> self._pre_launch()
> self._qemu_full_args = (self._wrapper + [self._binary] +
> @@ -284,14 +291,18 @@ class QEMUMachine(object):
> self._post_launch()
>
> def wait(self):
> -'''Wait for the VM to power off'''
> +"""
> +Wait for the VM to power off
> +"""
> self._popen.wait()
> self._qmp.close()
> self._load_io_log()
> self._post_shutdown()
>
> def shutdown(self):
> -'''Terminate the VM and clean up'''
> +"""
> +Terminate the VM and clean up
> +"""
> if self.is_running():
> try:
> self._qmp.cmd('quit')
> @@ -315,7 +326,9 @@ class QEMUMachine(object):
> self._launched = False
>
> def qmp(self, cmd, conv_keys=True, **args):
> -'''Invoke a QMP command and return the response dict'''
> +"""
> +Invoke a QMP command and return the response dict
> +"""
> qmp_args = dict()
> for key, value in args.items():
> if conv_keys:
> @@ -326,11 +339,11 @@ class QEMUMachine(object):
> return self._qmp.cmd(cmd, args=qmp_args)
>
> def command(self, cmd, conv_keys=True, **args):
> -'''
> +"""
> Invoke a QMP command.
> On success return the response dict.
> On failure raise an exception.
> -'''
> +"""
> reply = self.qmp(cmd, conv_keys, **args)
> if reply is None:
> raise qmp.qmp.QMPError("Monitor is closed")
> @@ -339,13 +352,17 @@ class QEMUMachine(object):
> return reply["return"]
>
> def get_qmp_event(self, wait=False):
> -'''Poll for one queued QMP events and return it'''
> +"""
> +Poll for one queued QMP events and return it
> +"""
> if len(self._events) > 0:
> return self._events.pop(0)
> return self._qmp.pull_event(wait=wait)
>
> def get_qmp_events(self, wait=False):
> -'''Poll for queued QMP events and return a list of dicts'''
> +"""
> +Poll for queued QMP events and return a
Re: [Qemu-devel] [Qemu-block] [PATCH v11 07/31] iotests.py: Add node_info()
On 10/05/2018 07:39 PM, Max Reitz wrote:
> This function queries a node; since we cannot do that right now, it
> executes query-named-block-nodes and returns the matching node's object.
>
> Signed-off-by: Max Reitz
> ---
> tests/qemu-iotests/iotests.py | 7 +++
> 1 file changed, 7 insertions(+)
>
> diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
> index 5c45788dac..604f200600 100644
> --- a/tests/qemu-iotests/iotests.py
> +++ b/tests/qemu-iotests/iotests.py
> @@ -465,6 +465,13 @@ class VM(qtest.QEMUQtestMachine):
> else:
> iotests.log(ev)
>
> +def node_info(self, node_name):
> +nodes = self.qmp('query-named-block-nodes')
> +for x in nodes['return']:
> +if x['node-name'] == node_name:
> +return x
> +return None
> +
>
> index_re = re.compile(r'([^\[]+)\[([^\]]+)\]')
>
>
Reviewed-by: John Snow
Do we ever want to revisit the idea that our API should not do any
pre-filtering and that it's always up to the client to do so?
--js
Re: [Qemu-devel] insecure git submodule URLs
On 7/15/18 7:56 PM, Jann Horn via Qemu-devel wrote: On Sun, Jul 15, 2018 at 11:18 PM Peter Maydell wrote: On 15 July 2018 at 20:50, Jann Horn via Qemu-devel wrote: I noticed that when I build QEMU from git for the first time, it pulls in submodules over the insecure git:// protocol - in other words, as far as I can tell, if I'm e.g. on an open wifi network while building QEMU for the first time, even if I cloned the main repository over https, anyone could smuggle in malicious code as part of e.g. a submodule's makefile. Yes, this came up the other week. I'm not sure what your preferred fix for this is, so I'm not sending a patch yet. As far as I can tell, the two options are: - change .gitmodules to use https for everything We should probably do this... As far as I can tell, the QEMU git server only supports the "dumb" git protocol when accessed over HTTPS, not the "smart" protocol. I'm not sure whether that might be why QEMU is currently still using the insecure git protocol instead of git over HTTPS? This is why we haven't switched over the submodules yet, yes. It's on Jeff's todo list for the server, though. Did we ever get this done? (And updating this thread to pull in Jeff's new email). (Reminded of this now that there is yet another submodule being proposed for mirroring) -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org
Re: [Qemu-devel] [PULL 0/8] softfloat queue
Adding Jeff and Stefan On 10/8/18 8:47 AM, Peter Maydell wrote: On 5 October 2018 at 19:01, Richard Henderson wrote: The following changes since commit ae7a4c0a4604bcfed40170db6cca576c44d872a2: Merge remote-tracking branch 'remotes/cohuck/tags/s390x-20181004' into staging (2018-10-05 16:05:06 +0100) are available in the Git repository at: https://github.com/rth7680/qemu.git tags/pull-fpu-20181005 for you to fetch changes up to 27ae5109a2ba8b6b679cce3e03e16570a34390a0: softfloat: Specialize udiv_qrnnd for ppc64 (2018-10-05 12:57:41 -0500) Testing infrastructure for softfpu (not run by default). Drop countLeadingZeros. Fix div_floats. Add udiv_qrnnd specializations for x86_64, s390x, ppc64 hosts. Applied, thanks. I guess we should mirror the new submodules on git.qemu.org. Yes, that's been our general practice. -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org
Re: [Qemu-devel] [PATCH v2 2/2] docs: Document vCPU hotplug procedure
On 10/1/18 3:59 AM, Igor Mammedov wrote: Anyway, what about this: The command returns an object with a "qom-path" member for each present CPU. In this case, it shows an IvyBridge-IBRS-x86_64-cpu in socket 0. It returns an object without a "qom-path" for every possibly CPU hot-plug. In this case, it shows you can plug an IvyBridge-IBRS-x86_64-cpu into socket 1, and the additional properties you need to pass to device_add for that. not really sure my English (CCed Eric) but to match 'an object' with the rest of sentence: It returns an object without a "qom-path" for a possible to hot-plug CPU. + In this case, it shows you can plug an IvyBridge-IBRS-x86_64-cpu into socket 1/core = 0/thread 0, where 'props' list describes additional properties you need to pass to device_add for hot-pluging that CPU. Maybe: The command returns an object for CPUs that are present (containing a "qom-path" member) or which may be hot-plugged (no "qom-path" member). In this example, an IvyBridge-IBRS-x86_64-cpu is present in socket 0, while hot-plugging a CPU into socket 1 requires passing the listed properties to device_add. -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org
Re: [Qemu-devel] [PATCH] sdl2: Support all virtio-gpu formats
On 08.10.18 20:50, Max Reitz wrote: > There are some 2D resource formats that can be used through virtio-gpu, > but which are not supported by SDL2 when used for a scanout; these are > all alpha-channel formats and also XBGR (RGBX in non-BE pixman). Oops, it's the other way round. The virtio-gpu format is RGBX, and the non-BE pixman (and SDL) constant is XBGR. Max > Add these formats in the switch converting pixman to SDL format > constants so a guest cannot crash the VM by triggering the > g_assert_not_reached() with an unsupported format. > > Signed-off-by: Max Reitz > --- > ui/sdl2-2d.c | 9 + > 1 file changed, 9 insertions(+) > > diff --git a/ui/sdl2-2d.c b/ui/sdl2-2d.c > index 85484407be..c9df72636a 100644 > --- a/ui/sdl2-2d.c > +++ b/ui/sdl2-2d.c > @@ -101,15 +101,24 @@ void sdl2_2d_switch(DisplayChangeListener *dcl, > case PIXMAN_r5g6b5: > format = SDL_PIXELFORMAT_RGB565; > break; > +case PIXMAN_a8r8g8b8: > case PIXMAN_x8r8g8b8: > format = SDL_PIXELFORMAT_ARGB; > break; > +case PIXMAN_a8b8g8r8: > +case PIXMAN_x8b8g8r8: > +format = SDL_PIXELFORMAT_ABGR; > +break; > +case PIXMAN_r8g8b8a8: > case PIXMAN_r8g8b8x8: > format = SDL_PIXELFORMAT_RGBA; > break; > case PIXMAN_b8g8r8x8: > format = SDL_PIXELFORMAT_BGRX; > break; > +case PIXMAN_b8g8r8a8: > +format = SDL_PIXELFORMAT_BGRA; > +break; > default: > g_assert_not_reached(); > } > signature.asc Description: OpenPGP digital signature
Re: [Qemu-devel] [PATCH 05/31] vfio: Clean up error reporting after previous commit
On Mon, 8 Oct 2018 19:30:59 +0200 Markus Armbruster wrote: > The previous commit changed vfio's warning messages from > > vfio warning: DEV-NAME: Could not frobnicate > > to > > warning: vfio DEV-NAME: Could not frobnicate > > To match this change, change error messages from > > vfio error: DEV-NAME: On fire > > to > > vfio DEV-NAME: On fire > > Note the loss of "error". If we think marking error messages that way > is a good idea, we should mark *all* error messages, i.e. make > error_report() print it. > > Cc: Alex Williamson > Signed-off-by: Markus Armbruster > --- > hw/vfio/pci-quirks.c | 4 ++-- > hw/vfio/pci.c | 8 > hw/vfio/platform.c| 2 +- > include/hw/vfio/vfio-common.h | 1 - > 4 files changed, 7 insertions(+), 8 deletions(-) Acked-by: Alex Williamson
Re: [Qemu-devel] [PATCH 04/31] vfio: Use warn_report() & friends to report warnings
On Mon, 8 Oct 2018 19:30:58 +0200 Markus Armbruster wrote: > The vfio code reports warnings like > > error_report(WARN_PREFIX "Could not frobnicate", DEV-NAME); > > where WARN_PREFIX is defined so the message comes out as > > vfio warning: DEV-NAME: Could not frobnicate > > This usage predates the introduction of warn_report() & friends in > commit 97f40301f1d. It's time to convert to that interface. Since > these functions already prefix the message with "warning: ", replace > WARN_PREFIX by VFIO_MSG_PREFIX, so the messages come out like > > warning: vfio DEV-NAME: Could not frobnicate > > The next commit will replace ERR_PREFIX. > > Cc: Alex Williamson > Signed-off-by: Markus Armbruster > --- > hw/vfio/pci.c | 14 +++--- > hw/vfio/platform.c| 4 ++-- > include/hw/vfio/vfio-common.h | 2 +- > 3 files changed, 10 insertions(+), 10 deletions(-) Acked-by: Alex Williamson
[Qemu-devel] [PATCH] sdl2: Support all virtio-gpu formats
There are some 2D resource formats that can be used through virtio-gpu, but which are not supported by SDL2 when used for a scanout; these are all alpha-channel formats and also XBGR (RGBX in non-BE pixman). Add these formats in the switch converting pixman to SDL format constants so a guest cannot crash the VM by triggering the g_assert_not_reached() with an unsupported format. Signed-off-by: Max Reitz --- ui/sdl2-2d.c | 9 + 1 file changed, 9 insertions(+) diff --git a/ui/sdl2-2d.c b/ui/sdl2-2d.c index 85484407be..c9df72636a 100644 --- a/ui/sdl2-2d.c +++ b/ui/sdl2-2d.c @@ -101,15 +101,24 @@ void sdl2_2d_switch(DisplayChangeListener *dcl, case PIXMAN_r5g6b5: format = SDL_PIXELFORMAT_RGB565; break; +case PIXMAN_a8r8g8b8: case PIXMAN_x8r8g8b8: format = SDL_PIXELFORMAT_ARGB; break; +case PIXMAN_a8b8g8r8: +case PIXMAN_x8b8g8r8: +format = SDL_PIXELFORMAT_ABGR; +break; +case PIXMAN_r8g8b8a8: case PIXMAN_r8g8b8x8: format = SDL_PIXELFORMAT_RGBA; break; case PIXMAN_b8g8r8x8: format = SDL_PIXELFORMAT_BGRX; break; +case PIXMAN_b8g8r8a8: +format = SDL_PIXELFORMAT_BGRA; +break; default: g_assert_not_reached(); } -- 2.17.1
Re: [Qemu-devel] [PATCH 14/14] block: Stop passing flags to bdrv_reopen_queue_child()
On 08.10.18 20:13, Alberto Garcia wrote:
> On Mon 08 Oct 2018 04:48:50 AM CEST, Max Reitz wrote:
>
>>> +/* Old values are used for options that aren't set yet */
>>> +old_options = qdict_clone_shallow(bs->options);
>>> +bdrv_join_options(bs, options, old_options);
>>> +qobject_unref(old_options);
>>> +
>>> +/* We have the final set of options so let's update the flags */
>>> +{
>>> +Error *local_err = NULL;
>>> +QemuOpts *opts;
>>> +QDict *options_copy = qdict_clone_shallow(options);
>>
>> I-I'm not sure this conforms to our coding style.
>>
>> While I applaud your effort to keep the patch size small, I know for
>> sure I don't want to read code like this. (Unless it's necessary
>> because of some variables' lifetimes.)
>
> I actually think it makes the code more readable if there are variables
> with a very limited scope (like in this case). But I can also rewrite it
> using a more traditional style.
I think it should be a function call then.
Max
signature.asc
Description: OpenPGP digital signature
Re: [Qemu-devel] [PATCH] tcg: Add tlb_index and tlb_entry helpers
On Sun, Oct 07, 2018 at 18:05:22 -0700, Richard Henderson wrote:
> Isolate the computation of an index from an address into a
> helper before we change that function.
>
> Signed-off-by: Richard Henderson
> ---
>
> Emilio, this should make your dynamic tlb sizing patch 1/6
> significantly smaller.
Nice! I'm adding this as patch 1 for v2. Had to fix the conflicts
with the .addr_write conversion to atomic_write, and also
fixed a few typos like the following:
> void helper_be_st_name(CPUArchState *env, target_ulong addr, DATA_TYPE val,
> TCGMemOpIdx oi, uintptr_t retaddr)
> {
> -unsigned mmu_idx = get_mmuidx(oi);
> -int index = (addr >> TARGET_PAGE_BITS) & (CPU_TLB_SIZE - 1);
> -target_ulong tlb_addr = env->tlb_table[mmu_idx][index].addr_write;
> +uintptr_t mmu_idx = get_mmuidx(oi);
> +uintptr_t index = tlb_index(env, mmu_idx, addr);
> +CPUTLBEntry *entry = tlb_entry(env, mmu_idx, index);
Should be tlb_entry(env, mmu_idx, addr)
Thanks,
Emilio
[Qemu-devel] [PATCH v1 4/5] RISC-V: Add missing free for plic_hart_config
From: Michael Clark Cc: Palmer Dabbelt Cc: Sagar Karandikar Cc: Bastian Koppelmann Cc: Alistair Francis Signed-off-by: Michael Clark Reviewed-by: Alistair Francis --- hw/riscv/virt.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/hw/riscv/virt.c b/hw/riscv/virt.c index 005169eabc..6bd723dc3a 100644 --- a/hw/riscv/virt.c +++ b/hw/riscv/virt.c @@ -385,6 +385,8 @@ static void riscv_virt_board_init(MachineState *machine) serial_mm_init(system_memory, memmap[VIRT_UART0].base, 0, qdev_get_gpio_in(DEVICE(s->plic), UART0_IRQ), 399193, serial_hd(0), DEVICE_LITTLE_ENDIAN); + +g_free(plic_hart_config); } static void riscv_virt_board_machine_init(MachineClass *mc) -- 2.17.1
[Qemu-devel] [PATCH v1 2/5] RISC-V: Move non-ops from op_helper to cpu_helper
From: Michael Clark
This patch makes op_helper.c contain only instruction
operation helpers used by translate.c and moves any
unrelated cpu helpers into cpu_helper.c. No logic is
changed by this patch.
Cc: Sagar Karandikar
Cc: Bastian Koppelmann
Cc: Palmer Dabbelt
Cc: Alistair Francis
Signed-off-by: Michael Clark
Reviewed-by: Alistair Francis
---
target/riscv/Makefile.objs | 2 +-
target/riscv/{helper.c => cpu_helper.c} | 35 -
target/riscv/op_helper.c| 34
3 files changed, 35 insertions(+), 36 deletions(-)
rename target/riscv/{helper.c => cpu_helper.c} (95%)
diff --git a/target/riscv/Makefile.objs b/target/riscv/Makefile.objs
index abd0a7cde3..fcc5d34c1f 100644
--- a/target/riscv/Makefile.objs
+++ b/target/riscv/Makefile.objs
@@ -1 +1 @@
-obj-y += translate.o op_helper.o helper.o cpu.o fpu_helper.o gdbstub.o pmp.o
+obj-y += translate.o op_helper.o cpu_helper.o cpu.o fpu_helper.o gdbstub.o
pmp.o
diff --git a/target/riscv/helper.c b/target/riscv/cpu_helper.c
similarity index 95%
rename from target/riscv/helper.c
rename to target/riscv/cpu_helper.c
index 63b3386b76..86f9f4730c 100644
--- a/target/riscv/helper.c
+++ b/target/riscv/cpu_helper.c
@@ -1,5 +1,5 @@
/*
- * RISC-V emulation helpers for qemu.
+ * RISC-V CPU helpers for qemu.
*
* Copyright (c) 2016-2017 Sagar Karandikar, sag...@eecs.berkeley.edu
* Copyright (c) 2017-2018 SiFive, Inc.
@@ -72,6 +72,39 @@ bool riscv_cpu_exec_interrupt(CPUState *cs, int
interrupt_request)
#if !defined(CONFIG_USER_ONLY)
+/* iothread_mutex must be held */
+uint32_t riscv_cpu_update_mip(RISCVCPU *cpu, uint32_t mask, uint32_t value)
+{
+CPURISCVState *env = >env;
+uint32_t old, new, cmp = atomic_read(>mip);
+
+do {
+old = cmp;
+new = (old & ~mask) | (value & mask);
+cmp = atomic_cmpxchg(>mip, old, new);
+} while (old != cmp);
+
+if (new && !old) {
+cpu_interrupt(CPU(cpu), CPU_INTERRUPT_HARD);
+} else if (!new && old) {
+cpu_reset_interrupt(CPU(cpu), CPU_INTERRUPT_HARD);
+}
+
+return old;
+}
+
+void riscv_set_mode(CPURISCVState *env, target_ulong newpriv)
+{
+if (newpriv > PRV_M) {
+g_assert_not_reached();
+}
+if (newpriv == PRV_H) {
+newpriv = PRV_U;
+}
+/* tlb_flush is unnecessary as mode is contained in mmu_idx */
+env->priv = newpriv;
+}
+
/* get_physical_address - get the physical address for this virtual address
*
* Do a page table walk to obtain the physical address corresponding to a
diff --git a/target/riscv/op_helper.c b/target/riscv/op_helper.c
index d0883d329b..495390ab1c 100644
--- a/target/riscv/op_helper.c
+++ b/target/riscv/op_helper.c
@@ -654,39 +654,6 @@ target_ulong helper_csrrc(CPURISCVState *env, target_ulong
src,
#ifndef CONFIG_USER_ONLY
-/* iothread_mutex must be held */
-uint32_t riscv_cpu_update_mip(RISCVCPU *cpu, uint32_t mask, uint32_t value)
-{
-CPURISCVState *env = >env;
-uint32_t old, new, cmp = atomic_read(>mip);
-
-do {
-old = cmp;
-new = (old & ~mask) | (value & mask);
-cmp = atomic_cmpxchg(>mip, old, new);
-} while (old != cmp);
-
-if (new && !old) {
-cpu_interrupt(CPU(cpu), CPU_INTERRUPT_HARD);
-} else if (!new && old) {
-cpu_reset_interrupt(CPU(cpu), CPU_INTERRUPT_HARD);
-}
-
-return old;
-}
-
-void riscv_set_mode(CPURISCVState *env, target_ulong newpriv)
-{
-if (newpriv > PRV_M) {
-g_assert_not_reached();
-}
-if (newpriv == PRV_H) {
-newpriv = PRV_U;
-}
-/* tlb_flush is unnecessary as mode is contained in mmu_idx */
-env->priv = newpriv;
-}
-
target_ulong helper_sret(CPURISCVState *env, target_ulong cpu_pc_deb)
{
if (!(env->priv >= PRV_S)) {
@@ -737,7 +704,6 @@ target_ulong helper_mret(CPURISCVState *env, target_ulong
cpu_pc_deb)
return retpc;
}
-
void helper_wfi(CPURISCVState *env)
{
CPUState *cs = CPU(riscv_env_get_cpu(env));
--
2.17.1
[Qemu-devel] [PATCH v1 5/5] RISC-V: Don't add NULL bootargs to device-tree
From: Michael Clark
Cc: Palmer Dabbelt
Cc: Alistair Francis
Signed-off-by: Michael Clark
Reviewed-by: Alistair Francis
---
hw/riscv/sifive_u.c | 4 +++-
hw/riscv/spike.c| 6 --
hw/riscv/virt.c | 4 +++-
3 files changed, 10 insertions(+), 4 deletions(-)
diff --git a/hw/riscv/sifive_u.c b/hw/riscv/sifive_u.c
index 862f8ff5f7..ef07df2442 100644
--- a/hw/riscv/sifive_u.c
+++ b/hw/riscv/sifive_u.c
@@ -230,7 +230,9 @@ static void create_fdt(SiFiveUState *s, const struct
MemmapEntry *memmap,
qemu_fdt_add_subnode(fdt, "/chosen");
qemu_fdt_setprop_string(fdt, "/chosen", "stdout-path", nodename);
-qemu_fdt_setprop_string(fdt, "/chosen", "bootargs", cmdline);
+if (cmdline) {
+qemu_fdt_setprop_string(fdt, "/chosen", "bootargs", cmdline);
+}
g_free(nodename);
}
diff --git a/hw/riscv/spike.c b/hw/riscv/spike.c
index be5ef85e81..8a712ed490 100644
--- a/hw/riscv/spike.c
+++ b/hw/riscv/spike.c
@@ -156,8 +156,10 @@ static void create_fdt(SpikeState *s, const struct
MemmapEntry *memmap,
g_free(cells);
g_free(nodename);
-qemu_fdt_add_subnode(fdt, "/chosen");
-qemu_fdt_setprop_string(fdt, "/chosen", "bootargs", cmdline);
+if (cmdline) {
+qemu_fdt_add_subnode(fdt, "/chosen");
+qemu_fdt_setprop_string(fdt, "/chosen", "bootargs", cmdline);
+}
}
static void spike_v1_10_0_board_init(MachineState *machine)
diff --git a/hw/riscv/virt.c b/hw/riscv/virt.c
index 6bd723dc3a..4a137a503c 100644
--- a/hw/riscv/virt.c
+++ b/hw/riscv/virt.c
@@ -254,7 +254,9 @@ static void *create_fdt(RISCVVirtState *s, const struct
MemmapEntry *memmap,
qemu_fdt_add_subnode(fdt, "/chosen");
qemu_fdt_setprop_string(fdt, "/chosen", "stdout-path", nodename);
-qemu_fdt_setprop_string(fdt, "/chosen", "bootargs", cmdline);
+if (cmdline) {
+qemu_fdt_setprop_string(fdt, "/chosen", "bootargs", cmdline);
+}
g_free(nodename);
return fdt;
--
2.17.1
[Qemu-devel] [PATCH v1 3/5] RISC-V: Update CSR and interrupt definitions
From: Michael Clark
* Add user-mode CSR defininitions.
* Reorder CSR definitions to match the specification.
* Change H mode interrupt comment to 'reserved'.
* Remove unused X_COP interrupt.
* Add user-mode interrupts.
* Remove erroneous until comments on machine mode interrupts.
* Move together paging mode and page table bit definitions.
* Move together interrupt and exception cause definitions.
Cc: Sagar Karandikar
Cc: Bastian Koppelmann
Cc: Palmer Dabbelt
Cc: Alistair Francis
Signed-off-by: Michael Clark
Reviewed-by: Alistair Francis
---
target/riscv/cpu.c | 6 +-
target/riscv/cpu_bits.h | 683 +--
target/riscv/op_helper.c | 2 +-
3 files changed, 370 insertions(+), 321 deletions(-)
diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
index d630e8fd6c..a025a0a3ba 100644
--- a/target/riscv/cpu.c
+++ b/target/riscv/cpu.c
@@ -74,8 +74,10 @@ const char * const riscv_intr_names[] = {
"s_external",
"h_external",
"m_external",
-"coprocessor",
-"host"
+"reserved",
+"reserved",
+"reserved",
+"reserved"
};
typedef struct RISCVCPUInfo {
diff --git a/target/riscv/cpu_bits.h b/target/riscv/cpu_bits.h
index 12b4757088..5439f4719e 100644
--- a/target/riscv/cpu_bits.h
+++ b/target/riscv/cpu_bits.h
@@ -6,242 +6,283 @@
(((target_ulong)(val) * ((mask) & ~((mask) << 1))) & \
(target_ulong)(mask)))
-#define PGSHIFT 12
-
-#define FSR_RD_SHIFT 5
-#define FSR_RD (0x7 << FSR_RD_SHIFT)
-
-#define FPEXC_NX 0x01
-#define FPEXC_UF 0x02
-#define FPEXC_OF 0x04
-#define FPEXC_DZ 0x08
-#define FPEXC_NV 0x10
-
-#define FSR_AEXC_SHIFT 0
-#define FSR_NVA (FPEXC_NV << FSR_AEXC_SHIFT)
-#define FSR_OFA (FPEXC_OF << FSR_AEXC_SHIFT)
-#define FSR_UFA (FPEXC_UF << FSR_AEXC_SHIFT)
-#define FSR_DZA (FPEXC_DZ << FSR_AEXC_SHIFT)
-#define FSR_NXA (FPEXC_NX << FSR_AEXC_SHIFT)
-#define FSR_AEXC (FSR_NVA | FSR_OFA | FSR_UFA | FSR_DZA | FSR_NXA)
-
-/* CSR numbers */
-#define CSR_FFLAGS 0x1
-#define CSR_FRM 0x2
-#define CSR_FCSR 0x3
-#define CSR_CYCLE 0xc00
-#define CSR_TIME 0xc01
-#define CSR_INSTRET 0xc02
-#define CSR_HPMCOUNTER3 0xc03
-#define CSR_HPMCOUNTER4 0xc04
-#define CSR_HPMCOUNTER5 0xc05
-#define CSR_HPMCOUNTER6 0xc06
-#define CSR_HPMCOUNTER7 0xc07
-#define CSR_HPMCOUNTER8 0xc08
-#define CSR_HPMCOUNTER9 0xc09
-#define CSR_HPMCOUNTER10 0xc0a
-#define CSR_HPMCOUNTER11 0xc0b
-#define CSR_HPMCOUNTER12 0xc0c
-#define CSR_HPMCOUNTER13 0xc0d
-#define CSR_HPMCOUNTER14 0xc0e
-#define CSR_HPMCOUNTER15 0xc0f
-#define CSR_HPMCOUNTER16 0xc10
-#define CSR_HPMCOUNTER17 0xc11
-#define CSR_HPMCOUNTER18 0xc12
-#define CSR_HPMCOUNTER19 0xc13
-#define CSR_HPMCOUNTER20 0xc14
-#define CSR_HPMCOUNTER21 0xc15
-#define CSR_HPMCOUNTER22 0xc16
-#define CSR_HPMCOUNTER23 0xc17
-#define CSR_HPMCOUNTER24 0xc18
-#define CSR_HPMCOUNTER25 0xc19
-#define CSR_HPMCOUNTER26 0xc1a
-#define CSR_HPMCOUNTER27 0xc1b
-#define CSR_HPMCOUNTER28 0xc1c
-#define CSR_HPMCOUNTER29 0xc1d
-#define CSR_HPMCOUNTER30 0xc1e
-#define CSR_HPMCOUNTER31 0xc1f
-#define CSR_SSTATUS 0x100
-#define CSR_SIE 0x104
-#define CSR_STVEC 0x105
-#define CSR_SCOUNTEREN 0x106
-#define CSR_SSCRATCH 0x140
-#define CSR_SEPC 0x141
-#define CSR_SCAUSE 0x142
-#define CSR_SBADADDR 0x143
-#define CSR_SIP 0x144
-#define CSR_SPTBR 0x180
-#define CSR_SATP 0x180
-#define CSR_MSTATUS 0x300
-#define CSR_MISA 0x301
-#define CSR_MEDELEG 0x302
-#define CSR_MIDELEG 0x303
-#define CSR_MIE 0x304
-#define CSR_MTVEC 0x305
-#define CSR_MCOUNTEREN 0x306
-#define CSR_MSCRATCH 0x340
-#define CSR_MEPC 0x341
-#define CSR_MCAUSE 0x342
-#define CSR_MBADADDR 0x343
-#define CSR_MIP 0x344
-#define CSR_PMPCFG0 0x3a0
-#define CSR_PMPCFG1 0x3a1
-#define CSR_PMPCFG2 0x3a2
-#define CSR_PMPCFG3 0x3a3
-#define CSR_PMPADDR0 0x3b0
-#define CSR_PMPADDR1 0x3b1
-#define CSR_PMPADDR2 0x3b2
-#define CSR_PMPADDR3 0x3b3
-#define CSR_PMPADDR4 0x3b4
-#define CSR_PMPADDR5 0x3b5
-#define CSR_PMPADDR6 0x3b6
-#define CSR_PMPADDR7 0x3b7
-#define CSR_PMPADDR8 0x3b8
-#define CSR_PMPADDR9 0x3b9
-#define CSR_PMPADDR10 0x3ba
-#define CSR_PMPADDR11 0x3bb
-#define CSR_PMPADDR12 0x3bc
-#define CSR_PMPADDR13 0x3bd
-#define CSR_PMPADDR14 0x3be
-#define CSR_PMPADDR15 0x3bf
-#define CSR_TSELECT 0x7a0
-#define CSR_TDATA1 0x7a1
-#define CSR_TDATA2 0x7a2
-#define CSR_TDATA3 0x7a3
-#define CSR_DCSR 0x7b0
-#define CSR_DPC 0x7b1
-#define CSR_DSCRATCH 0x7b2
-#define CSR_MCYCLE 0xb00
-#define CSR_MINSTRET 0xb02
-#define CSR_MHPMCOUNTER3 0xb03
-#define CSR_MHPMCOUNTER4 0xb04
-#define CSR_MHPMCOUNTER5 0xb05
-#define CSR_MHPMCOUNTER6 0xb06
-#define CSR_MHPMCOUNTER7 0xb07
-#define CSR_MHPMCOUNTER8 0xb08
-#define CSR_MHPMCOUNTER9 0xb09
-#define CSR_MHPMCOUNTER10 0xb0a
-#define CSR_MHPMCOUNTER11 0xb0b
-#define CSR_MHPMCOUNTER12 0xb0c
-#define CSR_MHPMCOUNTER13 0xb0d
-#define CSR_MHPMCOUNTER14 0xb0e
-#define CSR_MHPMCOUNTER15 0xb0f
-#define CSR_MHPMCOUNTER16 0xb10
-#define CSR_MHPMCOUNTER17 0xb11
-#define CSR_MHPMCOUNTER18 0xb12
-#define CSR_MHPMCOUNTER19
[Qemu-devel] [PATCH v1 0/5] Misc RISC-V patches
These are some patches that I have cherry picked from Michael's RISC-V
tree that are ready to be applied.
Unless anyone has any comments against these I'll send a PR later this
week.
Michael Clark (5):
RISC-V: Allow setting and clearing multiple irqs
RISC-V: Move non-ops from op_helper to cpu_helper
RISC-V: Update CSR and interrupt definitions
RISC-V: Add missing free for plic_hart_config
RISC-V: Don't add NULL bootargs to device-tree
hw/riscv/sifive_clint.c | 8 +-
hw/riscv/sifive_plic.c | 4 +-
hw/riscv/sifive_u.c | 4 +-
hw/riscv/spike.c| 6 +-
hw/riscv/virt.c | 6 +-
target/riscv/Makefile.objs | 2 +-
target/riscv/cpu.c | 6 +-
target/riscv/cpu.h | 22 +-
target/riscv/cpu_bits.h | 683 +---
target/riscv/{helper.c => cpu_helper.c} | 35 +-
target/riscv/op_helper.c| 34 +-
11 files changed, 438 insertions(+), 372 deletions(-)
rename target/riscv/{helper.c => cpu_helper.c} (95%)
--
2.17.1
[Qemu-devel] [PATCH v1 1/5] RISC-V: Allow setting and clearing multiple irqs
From: Michael Clark
Change the API of riscv_set_local_interrupt to take a
write mask and value to allow setting and clearing of
multiple local interrupts atomically in a single call.
Rename the new function to riscv_cpu_update_mip.
Cc: Sagar Karandikar
Cc: Bastian Koppelmann
Cc: Palmer Dabbelt
Cc: Alistair Francis
Signed-off-by: Michael Clark
Reviewed-by: Alistair Francis
---
hw/riscv/sifive_clint.c | 8
hw/riscv/sifive_plic.c | 4 ++--
target/riscv/cpu.h | 22 +-
target/riscv/op_helper.c | 24 +++-
4 files changed, 34 insertions(+), 24 deletions(-)
diff --git a/hw/riscv/sifive_clint.c b/hw/riscv/sifive_clint.c
index 7cc606e065..0d2fd52487 100644
--- a/hw/riscv/sifive_clint.c
+++ b/hw/riscv/sifive_clint.c
@@ -47,12 +47,12 @@ static void sifive_clint_write_timecmp(RISCVCPU *cpu,
uint64_t value)
if (cpu->env.timecmp <= rtc_r) {
/* if we're setting an MTIMECMP value in the "past",
immediately raise the timer interrupt */
-riscv_set_local_interrupt(cpu, MIP_MTIP, 1);
+riscv_cpu_update_mip(cpu, MIP_MTIP, BOOL_TO_MASK(1));
return;
}
/* otherwise, set up the future timer interrupt */
-riscv_set_local_interrupt(cpu, MIP_MTIP, 0);
+riscv_cpu_update_mip(cpu, MIP_MTIP, BOOL_TO_MASK(0));
diff = cpu->env.timecmp - rtc_r;
/* back to ns (note args switched in muldiv64) */
next = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL) +
@@ -67,7 +67,7 @@ static void sifive_clint_write_timecmp(RISCVCPU *cpu,
uint64_t value)
static void sifive_clint_timer_cb(void *opaque)
{
RISCVCPU *cpu = opaque;
-riscv_set_local_interrupt(cpu, MIP_MTIP, 1);
+riscv_cpu_update_mip(cpu, MIP_MTIP, BOOL_TO_MASK(1));
}
/* CPU wants to read rtc or timecmp register */
@@ -132,7 +132,7 @@ static void sifive_clint_write(void *opaque, hwaddr addr,
uint64_t value,
if (!env) {
error_report("clint: invalid timecmp hartid: %zu", hartid);
} else if ((addr & 0x3) == 0) {
-riscv_set_local_interrupt(RISCV_CPU(cpu), MIP_MSIP, value != 0);
+riscv_cpu_update_mip(RISCV_CPU(cpu), MIP_MSIP,
BOOL_TO_MASK(value));
} else {
error_report("clint: invalid sip write: %08x", (uint32_t)addr);
}
diff --git a/hw/riscv/sifive_plic.c b/hw/riscv/sifive_plic.c
index f635e6ff67..9cf9a1f986 100644
--- a/hw/riscv/sifive_plic.c
+++ b/hw/riscv/sifive_plic.c
@@ -142,10 +142,10 @@ static void sifive_plic_update(SiFivePLICState *plic)
int level = sifive_plic_irqs_pending(plic, addrid);
switch (mode) {
case PLICMode_M:
-riscv_set_local_interrupt(RISCV_CPU(cpu), MIP_MEIP, level);
+riscv_cpu_update_mip(RISCV_CPU(cpu), MIP_MEIP,
BOOL_TO_MASK(level));
break;
case PLICMode_S:
-riscv_set_local_interrupt(RISCV_CPU(cpu), MIP_SEIP, level);
+riscv_cpu_update_mip(RISCV_CPU(cpu), MIP_SEIP,
BOOL_TO_MASK(level));
break;
default:
break;
diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h
index d4f36295f0..4ee09b9cff 100644
--- a/target/riscv/cpu.h
+++ b/target/riscv/cpu.h
@@ -126,13 +126,18 @@ struct CPURISCVState {
target_ulong mhartid;
target_ulong mstatus;
+
/*
* CAUTION! Unlike the rest of this struct, mip is accessed asynchonously
- * by I/O threads and other vCPUs, so hold the iothread mutex before
- * operating on it. CPU_INTERRUPT_HARD should be in effect iff this is
- * non-zero. Use riscv_cpu_set_local_interrupt.
+ * by I/O threads. It should be read with atomic_read. It should be updated
+ * using riscv_cpu_update_mip with the iothread mutex held. The iothread
+ * mutex must be held because mip must be consistent with the CPU inturrept
+ * state. riscv_cpu_update_mip calls cpu_interrupt or cpu_reset_interrupt
+ * wuth the invariant that CPU_INTERRUPT_HARD is set iff mip is non-zero.
+ * mip is 32-bits to allow atomic_read on 32-bit hosts.
*/
-uint32_t mip;/* allow atomic_read for >= 32-bit hosts */
+uint32_t mip;
+
target_ulong mie;
target_ulong mideleg;
@@ -247,7 +252,6 @@ void riscv_cpu_do_unaligned_access(CPUState *cs, vaddr
addr,
uintptr_t retaddr);
int riscv_cpu_handle_mmu_fault(CPUState *cpu, vaddr address, int size,
int rw, int mmu_idx);
-
char *riscv_isa_string(RISCVCPU *cpu);
void riscv_cpu_list(FILE *f, fprintf_function cpu_fprintf);
@@ -255,6 +259,10 @@ void riscv_cpu_list(FILE *f, fprintf_function cpu_fprintf);
#define cpu_list riscv_cpu_list
#define cpu_mmu_index riscv_cpu_mmu_index
+#ifndef CONFIG_USER_ONLY
+uint32_t riscv_cpu_update_mip(RISCVCPU *cpu, uint32_t mask, uint32_t value);
+#define BOOL_TO_MASK(x) (-!!(x)) /* helper for riscv_cpu_update_mip value */
+#endif
void riscv_set_mode(CPURISCVState *env, target_ulong
Re: [Qemu-devel] [PATCH v2 0/3] HMP/snapshot changes - do not use ID anymore
ping On 9/6/18 8:11 AM, Daniel Henrique Barboza wrote: changes in v2: - removed the "RFC" marker; - added a new patch (patch 2) that removes bdrv_snapshot_delete_by_id_or_name from the code; - made changes in patch 1 as suggested by Murilo; - previous patch set link: https://lists.gnu.org/archive/html/qemu-devel/2018-08/msg04658.html It is not uncommon to see bugs being opened by testers that attempt to create VM snapshots using HMP. It turns out that "0" and "1" are quite common snapshot names and they trigger a lot of bugs. I gave an example in the commit message of patch 1, but to sum up here: QEMU treats the input of savevm/loadvm/delvm sometimes as 'ID', sometimes as 'name'. It is documented as such, but this can lead to strange situations. Given that it is strange for an API to consider a parameter to be 2 fields at the same time, and inadvently treating them as one or the other, and that removing the ID field is too drastic, my idea here is to keep the ID field for internal control, but do not let the user set it. I guess there's room for discussion about considering this change an API change or not. It doesn't affect users of HMP and it doesn't affect Libvirt, but simplifying the meaning of the parameters of savevm/loadvm/delvm. Daniel Henrique Barboza (3): block/snapshot.c: eliminate use of ID input in snapshot operations block/snapshot: remove bdrv_snapshot_delete_by_id_or_name qcow2-snapshot: remove redundant find_snapshot_by_id_and_name call block/qcow2-snapshot.c | 5 - block/snapshot.c | 25 +++-- hmp-commands.hx | 24 include/block/snapshot.h | 3 --- qemu-img.c | 15 +++ 5 files changed, 26 insertions(+), 46 deletions(-)
Re: [Qemu-devel] [PATCH 14/14] block: Stop passing flags to bdrv_reopen_queue_child()
On Mon 08 Oct 2018 04:48:50 AM CEST, Max Reitz wrote:
>> +/* Old values are used for options that aren't set yet */
>> +old_options = qdict_clone_shallow(bs->options);
>> +bdrv_join_options(bs, options, old_options);
>> +qobject_unref(old_options);
>> +
>> +/* We have the final set of options so let's update the flags */
>> +{
>> +Error *local_err = NULL;
>> +QemuOpts *opts;
>> +QDict *options_copy = qdict_clone_shallow(options);
>
> I-I'm not sure this conforms to our coding style.
>
> While I applaud your effort to keep the patch size small, I know for
> sure I don't want to read code like this. (Unless it's necessary
> because of some variables' lifetimes.)
I actually think it makes the code more readable if there are variables
with a very limited scope (like in this case). But I can also rewrite it
using a more traditional style.
Berto
Re: [Qemu-devel] [PATCH v9 0/3] wakeup-from-suspend and system_wakeup changes
ping On 9/18/18 3:52 PM, Daniel Henrique Barboza wrote: changes in v9, all proposed by Mike Roth: - added a new 'qemu_register_wakeup_support' to be called by the wake-up implementations to register the support in vl.c (patch 1) - changed versions from 3.0.0 to 3.1.0 (patch 1) - added back the 'qemu_system_wakeup_request' call that was removed by mistake in the previous version (patch 3) - Previous series link: https://lists.gnu.org/archive/html/qemu-devel/2018-07/msg01675.html Daniel Henrique Barboza (3): qmp: query-current-machine with wakeup-suspend-support qga: update guest-suspend-ram and guest-suspend-hybrid descriptions qmp hmp: Make system_wakeup check wake-up support and run state hmp.c | 5 - hw/acpi/core.c | 1 + include/sysemu/sysemu.h | 2 ++ qapi/misc.json | 29 - qga/qapi-schema.json| 12 ++-- qmp.c | 10 ++ vl.c| 19 +++ 7 files changed, 70 insertions(+), 8 deletions(-)
[Qemu-devel] [PATCH v4 4/4] cputlb: read CPUTLBEntry.addr_write atomically
Updates can come from other threads, so readers that do not
take tlb_lock must use atomic_read to avoid undefined
behaviour (UB).
This and the previous commit result on average in no performance loss,
as the following experiments (run on an Intel i7-6700K CPU @ 4.00GHz)
show.
1. aarch64 bootup+shutdown test:
- Before:
Performance counter stats for 'taskset -c 0 ../img/aarch64/die.sh' (10 runs):
7487.087786 task-clock (msec) #0.998 CPUs utilized
( +- 0.12% )
31,574,905,303 cycles#4.217 GHz
( +- 0.12% )
57,097,908,812 instructions #1.81 insns per cycle
( +- 0.08% )
10,255,415,367 branches # 1369.747 M/sec
( +- 0.08% )
173,278,962 branch-misses #1.69% of all branches
( +- 0.18% )
7.504481349 seconds time elapsed
( +- 0.14% )
- After:
Performance counter stats for 'taskset -c 0 ../img/aarch64/die.sh' (10 runs):
7462.441328 task-clock (msec) #0.998 CPUs utilized
( +- 0.07% )
31,478,476,520 cycles#4.218 GHz
( +- 0.07% )
57,017,330,084 instructions #1.81 insns per cycle
( +- 0.05% )
10,251,929,667 branches # 1373.804 M/sec
( +- 0.05% )
173,023,787 branch-misses #1.69% of all branches
( +- 0.11% )
7.474970463 seconds time elapsed
( +- 0.07% )
2. SPEC06int:
SPEC06int (test set)
[Y axis: Speedup over master]
1.15
+-++--+--+--+--+--+---+--+--+--+--+--+--++-+
|
|
1.1 +-+.+++.+
tlb-lock-v2 (m+++x) +-+
|+++ | +++
tlb-lock-v3 (spinl|ck) |
|+++ | | ++++++ |
||
1.05
+-++++....|.+++.|..|.###+++...+++###.+-+
| ### ++#| # |# |# ***### +++### +++#+# |
+++ | #|####|
1
+-+++***+#+++#++#++#++#+*+*++##+#++###########+##+#+++-+
|*+* ##++# *** # *** # * *++# +# *| * # |# |#
##|##+## #|
0.95
+-+..*.*.##..#.*|*..#...#..#.*|*..#.*.*..#.*|.*.#.*++*.#.*++*+#..##+##.#..++#.#..+-+
|* * ## # *|* # # # *|* # * * # *++* # * * # * * # * |*
# ++# ## # *** #|
|* * # ++# # *+* # # # *|* # * * # * * # * * # * * # *++*
# # ++# # * * #|
0.9
+-+..*.*.#...|#..#.*.*..#.++#..#.*|*..#.*.*..#.*..*.#.*..*.#.*..*.#.*..*.#.*.|*.#...|#.#..*.*.#..+-+
|* * # *** # * * # |# # *+* # * * # * * # * * # * * # * *
# *++* # |# # * * #|
0.85
+-+..*.*.#..*|*..#.*.*..#.***..#.*.*..#.*.*..#.*..*.#.*..*.#.*..*.#.*..*.#.*..*.#..#..*.*.#..+-+
|* * # *+* # * * # *|* # * * # * * # * * # * * # * * # * *
# * * # * |* # * * #|
|* * # * * # * * # *+* # * * # * * # * * # * * # * * # * *
# * * # * |* # * * #|
0.8
+-+..*.*.#..*.*..#.*.*..#.*.*..#.*.*..#.*.*..#.*..*.#.*..*.#.*..*.#.*..*.#.*..*.#.*++*.#..*.*.#..+-+
|* * # * * # * * # * * # * * # * * # * * # * * # * * # * *
# * * # * * # * * #|
0.75
+-+--***##--***###-***###-***###-***###-***###-##-##-##-##-##-##--***##--+-+
400.perlben401.bzip2403.gcc429.m445.gob456.hmme45462.libqua464.h26471.omnet473483.xalancbmkgeomean
png: https://imgur.com/a/BHzpPTW
Notes:
- tlb-lock-v2 corresponds to an implementation with a mutex.
- tlb-lock-v3 is the current patch series, i.e. with a spinlock
and a single lock acquisition in tlb_set_page_with_attrs.
Signed-off-by: Emilio G. Cota
---
accel/tcg/softmmu_template.h | 16 ++--
include/exec/cpu_ldst.h | 2 +-
include/exec/cpu_ldst_template.h | 2 +-
accel/tcg/cputlb.c | 15 +--
4 files changed, 21 insertions(+), 14 deletions(-)
diff --git a/accel/tcg/softmmu_template.h b/accel/tcg/softmmu_template.h
index f060a693d4..1e50263871 100644
--- a/accel/tcg/softmmu_template.h
+++ b/accel/tcg/softmmu_template.h
@@ -277,7 +277,8 @@ void helper_le_st_name(CPUArchState *env, target_ulong
addr, DATA_TYPE val,
{
unsigned mmu_idx = get_mmuidx(oi);
int index = (addr >>
[Qemu-devel] [PATCH v4 2/4] cputlb: fix assert_cpu_is_self macro
Reviewed-by: Richard Henderson
Reviewed-by: Alex Bennée
Signed-off-by: Emilio G. Cota
---
accel/tcg/cputlb.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/accel/tcg/cputlb.c b/accel/tcg/cputlb.c
index 502eea2850..f6b388c961 100644
--- a/accel/tcg/cputlb.c
+++ b/accel/tcg/cputlb.c
@@ -58,9 +58,9 @@
} \
} while (0)
-#define assert_cpu_is_self(this_cpu) do { \
+#define assert_cpu_is_self(cpu) do { \
if (DEBUG_TLB_GATE) { \
-g_assert(!cpu->created || qemu_cpu_is_self(cpu)); \
+g_assert(!(cpu)->created || qemu_cpu_is_self(cpu)); \
} \
} while (0)
--
2.17.1
[Qemu-devel] [PATCH v4 1/4] exec: introduce tlb_init
Paves the way for the addition of a per-TLB lock.
Reviewed-by: Alex Bennée
Signed-off-by: Emilio G. Cota
---
include/exec/exec-all.h | 8
accel/tcg/cputlb.c | 4
exec.c | 1 +
3 files changed, 13 insertions(+)
diff --git a/include/exec/exec-all.h b/include/exec/exec-all.h
index 5f78125582..815e5b1e83 100644
--- a/include/exec/exec-all.h
+++ b/include/exec/exec-all.h
@@ -99,6 +99,11 @@ void cpu_address_space_init(CPUState *cpu, int asidx,
#if !defined(CONFIG_USER_ONLY) && defined(CONFIG_TCG)
/* cputlb.c */
+/**
+ * tlb_init - initialize a CPU's TLB
+ * @cpu: CPU whose TLB should be initialized
+ */
+void tlb_init(CPUState *cpu);
/**
* tlb_flush_page:
* @cpu: CPU whose TLB should be flushed
@@ -258,6 +263,9 @@ void tlb_set_page(CPUState *cpu, target_ulong vaddr,
void probe_write(CPUArchState *env, target_ulong addr, int size, int mmu_idx,
uintptr_t retaddr);
#else
+static inline void tlb_init(CPUState *cpu)
+{
+}
static inline void tlb_flush_page(CPUState *cpu, target_ulong addr)
{
}
diff --git a/accel/tcg/cputlb.c b/accel/tcg/cputlb.c
index f4702ce91f..502eea2850 100644
--- a/accel/tcg/cputlb.c
+++ b/accel/tcg/cputlb.c
@@ -73,6 +73,10 @@ QEMU_BUILD_BUG_ON(sizeof(target_ulong) >
sizeof(run_on_cpu_data));
QEMU_BUILD_BUG_ON(NB_MMU_MODES > 16);
#define ALL_MMUIDX_BITS ((1 << NB_MMU_MODES) - 1)
+void tlb_init(CPUState *cpu)
+{
+}
+
/* flush_all_helper: run fn across all cpus
*
* If the wait flag is set then the src cpu's helper will be queued as
diff --git a/exec.c b/exec.c
index d0821e69aa..4fd831ef06 100644
--- a/exec.c
+++ b/exec.c
@@ -965,6 +965,7 @@ void cpu_exec_realizefn(CPUState *cpu, Error **errp)
tcg_target_initialized = true;
cc->tcg_initialize();
}
+tlb_init(cpu);
#ifndef CONFIG_USER_ONLY
if (qdev_get_vmsd(DEVICE(cpu)) == NULL) {
--
2.17.1
[Qemu-devel] [PATCH v4 3/4] cputlb: serialize tlb updates with env->tlb_lock
Currently we rely on atomic operations for cross-CPU invalidations.
There are two cases that these atomics miss: cross-CPU invalidations
can race with either (1) vCPU threads flushing their TLB, which
happens via memset, or (2) vCPUs calling tlb_reset_dirty on their TLB,
which updates .addr_write with a regular store. This results in
undefined behaviour, since we're mixing regular and atomic ops
on concurrent accesses.
Fix it by using tlb_lock, a per-vCPU lock. All updaters of tlb_table
and the corresponding victim cache now hold the lock.
The readers that do not hold tlb_lock must use atomic reads when
reading .addr_write, since this field can be updated by other threads;
the conversion to atomic reads is done in the next patch.
Note that an alternative fix would be to expand the use of atomic ops.
However, in the case of TLB flushes this would have a huge performance
impact, since (1) TLB flushes can happen very frequently and (2) we
currently use a full memory barrier to flush each TLB entry, and a TLB
has many entries. Instead, acquiring the lock is barely slower than a
full memory barrier since it is uncontended, and with a single lock
acquisition we can flush the entire TLB.
Tested-by: Alex Bennée
Reviewed-by: Alex Bennée
Signed-off-by: Emilio G. Cota
---
include/exec/cpu-defs.h | 3 +
accel/tcg/cputlb.c | 155 ++--
2 files changed, 87 insertions(+), 71 deletions(-)
diff --git a/include/exec/cpu-defs.h b/include/exec/cpu-defs.h
index a171ffc1a4..4ff62f32bf 100644
--- a/include/exec/cpu-defs.h
+++ b/include/exec/cpu-defs.h
@@ -24,6 +24,7 @@
#endif
#include "qemu/host-utils.h"
+#include "qemu/thread.h"
#include "qemu/queue.h"
#ifdef CONFIG_TCG
#include "tcg-target.h"
@@ -142,6 +143,8 @@ typedef struct CPUIOTLBEntry {
#define CPU_COMMON_TLB \
/* The meaning of the MMU modes is defined in the target code. */ \
+/* tlb_lock serializes updates to tlb_table and tlb_v_table */ \
+QemuSpin tlb_lock; \
CPUTLBEntry tlb_table[NB_MMU_MODES][CPU_TLB_SIZE]; \
CPUTLBEntry tlb_v_table[NB_MMU_MODES][CPU_VTLB_SIZE]; \
CPUIOTLBEntry iotlb[NB_MMU_MODES][CPU_TLB_SIZE];\
diff --git a/accel/tcg/cputlb.c b/accel/tcg/cputlb.c
index f6b388c961..c2a6190674 100644
--- a/accel/tcg/cputlb.c
+++ b/accel/tcg/cputlb.c
@@ -75,6 +75,9 @@ QEMU_BUILD_BUG_ON(NB_MMU_MODES > 16);
void tlb_init(CPUState *cpu)
{
+CPUArchState *env = cpu->env_ptr;
+
+qemu_spin_init(>tlb_lock);
}
/* flush_all_helper: run fn across all cpus
@@ -129,8 +132,17 @@ static void tlb_flush_nocheck(CPUState *cpu)
atomic_set(>tlb_flush_count, env->tlb_flush_count + 1);
tlb_debug("(count: %zu)\n", tlb_flush_count());
+/*
+ * tlb_table/tlb_v_table updates from any thread must hold tlb_lock.
+ * However, updates from the owner thread (as is the case here; see the
+ * above assert_cpu_is_self) do not need atomic_set because all reads
+ * that do not hold the lock are performed by the same owner thread.
+ */
+qemu_spin_lock(>tlb_lock);
memset(env->tlb_table, -1, sizeof(env->tlb_table));
memset(env->tlb_v_table, -1, sizeof(env->tlb_v_table));
+qemu_spin_unlock(>tlb_lock);
+
cpu_tb_jmp_cache_clear(cpu);
env->vtlb_index = 0;
@@ -182,6 +194,7 @@ static void tlb_flush_by_mmuidx_async_work(CPUState *cpu,
run_on_cpu_data data)
tlb_debug("start: mmu_idx:0x%04lx\n", mmu_idx_bitmask);
+qemu_spin_lock(>tlb_lock);
for (mmu_idx = 0; mmu_idx < NB_MMU_MODES; mmu_idx++) {
if (test_bit(mmu_idx, _idx_bitmask)) {
@@ -191,6 +204,7 @@ static void tlb_flush_by_mmuidx_async_work(CPUState *cpu,
run_on_cpu_data data)
memset(env->tlb_v_table[mmu_idx], -1, sizeof(env->tlb_v_table[0]));
}
}
+qemu_spin_unlock(>tlb_lock);
cpu_tb_jmp_cache_clear(cpu);
@@ -247,19 +261,24 @@ static inline bool tlb_hit_page_anyprot(CPUTLBEntry
*tlb_entry,
tlb_hit_page(tlb_entry->addr_code, page);
}
-static inline void tlb_flush_entry(CPUTLBEntry *tlb_entry, target_ulong page)
+/* Called with tlb_lock held */
+static inline void tlb_flush_entry_locked(CPUTLBEntry *tlb_entry,
+ target_ulong page)
{
if (tlb_hit_page_anyprot(tlb_entry, page)) {
memset(tlb_entry, -1, sizeof(*tlb_entry));
}
}
-static inline void tlb_flush_vtlb_page(CPUArchState *env, int mmu_idx,
- target_ulong page)
+/* Called with tlb_lock held */
+static inline void tlb_flush_vtlb_page_locked(CPUArchState *env, int mmu_idx,
+ target_ulong page)
{
int k;
+
+assert_cpu_is_self(ENV_GET_CPU(env));
for (k = 0; k < CPU_VTLB_SIZE; k++) {
-tlb_flush_entry(>tlb_v_table[mmu_idx][k], page);
+tlb_flush_entry_locked(>tlb_v_table[mmu_idx][k],
[Qemu-devel] [PATCH v4 0/4] per-TLB lock
v3: https://lists.gnu.org/archive/html/qemu-devel/2018-10/msg01087.html Changes since v3: - Add R-b's - Add comment to copy_tlb_helper_locked to note that it can only be called from the TLB owner thread. The series is checkpatch-clean. You can fetch it from: https://github.com/cota/qemu/tree/tlb-lock-v4 Thanks, Emilio
Re: [Qemu-devel] [PATCH 25/31] numa: Clean up error reporting in parse_numa()
On Mon, Oct 08, 2018 at 07:31:19PM +0200, Markus Armbruster wrote: > Calling error_report() in a function that takes an Error ** argument > is suspicious. parse_numa() does that, and then fails without setting > an error. Its caller main(), via qemu_opts_foreach(), is fine with > it, but clean it up anyway. > > While there, give parse_numa() internal linkage. > > Cc: Eduardo Habkost > Signed-off-by: Markus Armbruster Reviewed-by: Eduardo Habkost Queueing on machine-next. Thanks! -- Eduardo
Re: [Qemu-devel] [PATCH 14/31] numa: Fix QMP command set-numa-node error handling
On Mon, Oct 08, 2018 at 07:31:08PM +0200, Markus Armbruster wrote:
> Calling error_report() in a function that takes an Error ** argument
> is suspicious. parse_numa_node() does that, and then exit()s. It
> also passes _fatal to machine_set_cpu_numa_node(). Both wrong.
> Attempting to configure numa when the machine doesn't support it kills
> the VM:
>
> $ qemu-system-x86_64 -nodefaults -S -display none -M none -preconfig -qmp
> stdio
> {"QMP": {"version": {"qemu": {"micro": 50, "minor": 0, "major": 3},
> "package": "v3.0.0-837-gc5e4e49258"}, "capabilities": []}}
> {"execute": "qmp_capabilities"}
> {"return": {}}
> {"execute": "set-numa-node", "arguments": {"type": "node"}}
> NUMA is not supported by this machine-type
> $ echo $?
> 1
>
> Messed up when commit 64c2a8f6d3f and 7c88e65d9e9 (v2.10.0) added
> incorrect error handling right next to correct examples. Latent bug
> until commit f3be67812c2 (v3.0.0) made it accessible via QMP. Fairly
> harmless in practice, because it's limited to RUN_STATE_PRECONFIG.
> The fix is obvious: replace error_report(); exit() by error_setg();
> return.
>
> This affects parse_numa_node()'s other caller
> numa_complete_configuration(): since it ignores errors, the "NUMA is
> not supported by this machine-type" is now ignored, too. But that
> error is as unexpected there as any other. Change it to abort on
> error instead.
>
> Fixes: f3be67812c226162f86ce92634bd913714445420
> Cc: Igor Mammedov
> Signed-off-by: Markus Armbruster
>
> fixup! numa: Fix QMP command set-numa-node error handling
I assume this last line was kept by mistake, and I'm removing it
while committing.
--
Eduardo
[Qemu-devel] [RFC v1 1/1] qemu-iotests: Fix output for testcase 082
Commit 9cbef9d68ee: qemu-option: improve qemu_opts_print_help() output, changed some of the output for qemu-img tool but did not update the corresponding 082 test case file. This broke qemu-iotests. Signed-off-by: Farhan Ali --- tests/qemu-iotests/082.out | 956 ++--- 1 file changed, 478 insertions(+), 478 deletions(-) diff --git a/tests/qemu-iotests/082.out b/tests/qemu-iotests/082.out index 19e9fb1..2672349 100644 --- a/tests/qemu-iotests/082.out +++ b/tests/qemu-iotests/082.out @@ -44,171 +44,171 @@ cluster_size: 8192 Testing: create -f qcow2 -o help TEST_DIR/t.qcow2 128M Supported options: -size Virtual disk size -compat Compatibility level (0.10 or 1.1) -backing_file File name of a base image -backing_fmt Image format of the base image -encryption Encrypt the image with format 'aes'. (Deprecated in favor of encrypt.format=aes) -encrypt.format Encrypt the image, format choices: 'aes', 'luks' -encrypt.key-secret ID of secret providing qcow AES key or LUKS passphrase -encrypt.cipher-alg Name of encryption cipher algorithm -encrypt.cipher-mode Name of encryption cipher mode -encrypt.ivgen-alg Name of IV generator algorithm -encrypt.ivgen-hash-alg Name of IV generator hash algorithm -encrypt.hash-alg Name of encryption hash algorithm -encrypt.iter-time Time to spend in PBKDF in milliseconds -cluster_size qcow2 cluster size -preallocationPreallocation mode (allowed values: off, metadata, falloc, full) -lazy_refcounts Postpone refcount updates -refcount_bitsWidth of a reference count entry in bits -nocowTurn off copy-on-write (valid only on btrfs) +backing_file=str - File name of a base image +backing_fmt=str - Image format of the base image +cluster_size=size - qcow2 cluster size +compat=str - Compatibility level (0.10 or 1.1) +encrypt.cipher-alg=str - Name of encryption cipher algorithm +encrypt.cipher-mode=str - Name of encryption cipher mode +encrypt.format=str - Encrypt the image, format choices: 'aes', 'luks' +encrypt.hash-alg=str - Name of encryption hash algorithm +encrypt.iter-time=num - Time to spend in PBKDF in milliseconds +encrypt.ivgen-alg=str - Name of IV generator algorithm +encrypt.ivgen-hash-alg=str - Name of IV generator hash algorithm +encrypt.key-secret=str - ID of secret providing qcow AES key or LUKS passphrase +encryption=bool (on/off) - Encrypt the image with format 'aes'. (Deprecated in favor of encrypt.format=aes) +lazy_refcounts=bool (on/off) - Postpone refcount updates +nocow=bool (on/off) - Turn off copy-on-write (valid only on btrfs) +preallocation=str - Preallocation mode (allowed values: off, metadata, falloc, full) +refcount_bits=num - Width of a reference count entry in bits +size=size - Virtual disk size Testing: create -f qcow2 -o ? TEST_DIR/t.qcow2 128M Supported options: -size Virtual disk size -compat Compatibility level (0.10 or 1.1) -backing_file File name of a base image -backing_fmt Image format of the base image -encryption Encrypt the image with format 'aes'. (Deprecated in favor of encrypt.format=aes) -encrypt.format Encrypt the image, format choices: 'aes', 'luks' -encrypt.key-secret ID of secret providing qcow AES key or LUKS passphrase -encrypt.cipher-alg Name of encryption cipher algorithm -encrypt.cipher-mode Name of encryption cipher mode -encrypt.ivgen-alg Name of IV generator algorithm -encrypt.ivgen-hash-alg Name of IV generator hash algorithm -encrypt.hash-alg Name of encryption hash algorithm -encrypt.iter-time Time to spend in PBKDF in milliseconds -cluster_size qcow2 cluster size -preallocationPreallocation mode (allowed values: off, metadata, falloc, full) -lazy_refcounts Postpone refcount updates -refcount_bitsWidth of a reference count entry in bits -nocowTurn off copy-on-write (valid only on btrfs) +backing_file=str - File name of a base image +backing_fmt=str - Image format of the base image +cluster_size=size - qcow2 cluster size +compat=str - Compatibility level (0.10 or 1.1) +encrypt.cipher-alg=str - Name of encryption cipher algorithm +encrypt.cipher-mode=str - Name of encryption cipher mode +encrypt.format=str - Encrypt the image, format choices: 'aes', 'luks' +encrypt.hash-alg=str - Name of encryption hash algorithm +encrypt.iter-time=num - Time to spend in PBKDF in milliseconds +encrypt.ivgen-alg=str - Name of IV generator algorithm +encrypt.ivgen-hash-alg=str - Name of IV generator hash algorithm +encrypt.key-secret=str - ID of secret providing qcow AES key or LUKS passphrase +encryption=bool (on/off) - Encrypt the image with format 'aes'. (Deprecated in favor of encrypt.format=aes) +lazy_refcounts=bool (on/off) - Postpone refcount updates +nocow=bool (on/off) - Turn off copy-on-write (valid only on btrfs) +preallocation=str - Preallocation mode (allowed values: off, metadata, falloc, full) +refcount_bits=num - Width of a reference count entry in
Re: [Qemu-devel] [PATCH 08/14] block: Use bdrv_reopen_set_read_only() in external_snapshot_commit()
On Mon 08 Oct 2018 03:31:08 AM CEST, Max Reitz wrote: > On 19.09.18 16:47, Alberto Garcia wrote: >> This patch replaces the bdrv_reopen() calls that set and remove the >> BDRV_O_RDWR flag with the new bdrv_reopen_set_read_only() function. > > Ha! Got you! It's just one call this time, not "calls"! :-) :-D Berto
[Qemu-devel] [PATCH 30/31] blockdev: Convert drive_new() to Error
Calling error_report() from within a a function that takes an Error **
argument is suspicious. drive_new() does that, and its caller
drive_init_func() then exit()s. Its caller main(), via
qemu_opts_foreach(), is fine with it, but clean it up anyway:
* Convert drive_new() to Error
* Update add_init_drive() to report the error received from
drive_new().
* Make main() pass _fatal through qemu_opts_foreach(),
drive_init_func() to drive_new()
* Make default_drive() pass _abort through qemu_opts_foreach(),
drive_init_func() to drive_new()
Cc: Kevin Wolf
Cc: Max Reitz
Signed-off-by: Markus Armbruster
---
blockdev.c| 27 ++-
device-hotplug.c | 5 -
include/sysemu/blockdev.h | 3 ++-
vl.c | 11 ---
4 files changed, 24 insertions(+), 22 deletions(-)
diff --git a/blockdev.c b/blockdev.c
index a8755bd908..574adbcb7f 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -759,7 +759,8 @@ QemuOptsList qemu_legacy_drive_opts = {
},
};
-DriveInfo *drive_new(QemuOpts *all_opts, BlockInterfaceType block_default_type)
+DriveInfo *drive_new(QemuOpts *all_opts, BlockInterfaceType block_default_type,
+ Error **errp)
{
const char *value;
BlockBackend *blk;
@@ -808,7 +809,7 @@ DriveInfo *drive_new(QemuOpts *all_opts, BlockInterfaceType
block_default_type)
qemu_opt_rename(all_opts, opt_renames[i].from, opt_renames[i].to,
_err);
if (local_err) {
-error_report_err(local_err);
+error_propagate(errp, local_err);
return NULL;
}
}
@@ -819,7 +820,7 @@ DriveInfo *drive_new(QemuOpts *all_opts, BlockInterfaceType
block_default_type)
bool writethrough;
if (bdrv_parse_cache_mode(value, , ) != 0) {
-error_report("invalid cache option");
+error_setg(errp, "invalid cache option");
return NULL;
}
@@ -847,7 +848,7 @@ DriveInfo *drive_new(QemuOpts *all_opts, BlockInterfaceType
block_default_type)
_abort);
qemu_opts_absorb_qdict(legacy_opts, bs_opts, _err);
if (local_err) {
-error_report_err(local_err);
+error_propagate(errp, local_err);
goto fail;
}
@@ -860,7 +861,7 @@ DriveInfo *drive_new(QemuOpts *all_opts, BlockInterfaceType
block_default_type)
media = MEDIA_CDROM;
read_only = true;
} else {
-error_report("'%s' invalid media", value);
+error_setg(errp, "'%s' invalid media", value);
goto fail;
}
}
@@ -885,7 +886,7 @@ DriveInfo *drive_new(QemuOpts *all_opts, BlockInterfaceType
block_default_type)
type++) {
}
if (type == IF_COUNT) {
-error_report("unsupported bus type '%s'", value);
+error_setg(errp, "unsupported bus type '%s'", value);
goto fail;
}
} else {
@@ -902,7 +903,7 @@ DriveInfo *drive_new(QemuOpts *all_opts, BlockInterfaceType
block_default_type)
if (index != -1) {
if (bus_id != 0 || unit_id != -1) {
-error_report("index cannot be used with bus and unit");
+error_setg(errp, "index cannot be used with bus and unit");
goto fail;
}
bus_id = drive_index_to_bus_id(type, index);
@@ -921,13 +922,13 @@ DriveInfo *drive_new(QemuOpts *all_opts,
BlockInterfaceType block_default_type)
}
if (max_devs && unit_id >= max_devs) {
-error_report("unit %d too big (max is %d)", unit_id, max_devs - 1);
+error_setg(errp, "unit %d too big (max is %d)", unit_id, max_devs - 1);
goto fail;
}
if (drive_get(type, bus_id, unit_id) != NULL) {
-error_report("drive with bus=%d, unit=%d (index=%d) exists",
- bus_id, unit_id, index);
+error_setg(errp, "drive with bus=%d, unit=%d (index=%d) exists",
+ bus_id, unit_id, index);
goto fail;
}
@@ -970,7 +971,7 @@ DriveInfo *drive_new(QemuOpts *all_opts, BlockInterfaceType
block_default_type)
if (werror != NULL) {
if (type != IF_IDE && type != IF_SCSI && type != IF_VIRTIO &&
type != IF_NONE) {
-error_report("werror is not supported by this bus type");
+error_setg(errp, "werror is not supported by this bus type");
goto fail;
}
qdict_put_str(bs_opts, "werror", werror);
@@ -980,7 +981,7 @@ DriveInfo *drive_new(QemuOpts *all_opts, BlockInterfaceType
block_default_type)
if (rerror != NULL) {
if (type != IF_IDE && type != IF_VIRTIO && type != IF_SCSI &&
type != IF_NONE) {
-error_report("rerror is not supported by this bus type");
+error_setg(errp, "rerror is not supported by this bus type");
goto fail;
}
qdict_put_str(bs_opts, "rerror", rerror);
@@
[Qemu-devel] [PATCH 22/31] vl: Clean up error reporting in parse_fw_cfg()
Calling error_report() in a function that takes an Error ** argument
is suspicious. parse_fw_cfg() does that, and then fails without
setting an error. Its caller main(), via qemu_opts_foreach(), is fine
with it, but clean it up anyway.
Signed-off-by: Markus Armbruster
---
vl.c | 17 -
1 file changed, 8 insertions(+), 9 deletions(-)
diff --git a/vl.c b/vl.c
index 1009d708a0..a3a39ec06b 100644
--- a/vl.c
+++ b/vl.c
@@ -2174,7 +2174,7 @@ static int parse_fw_cfg(void *opaque, QemuOpts *opts,
Error **errp)
FWCfgState *fw_cfg = (FWCfgState *) opaque;
if (fw_cfg == NULL) {
-error_report("fw_cfg device not available");
+error_setg(errp, "fw_cfg device not available");
return -1;
}
name = qemu_opt_get(opts, "name");
@@ -2183,15 +2183,16 @@ static int parse_fw_cfg(void *opaque, QemuOpts *opts,
Error **errp)
/* we need name and either a file or the content string */
if (!(nonempty_str(name) && (nonempty_str(file) || nonempty_str(str {
-error_report("invalid argument(s)");
+error_setg(errp, "invalid argument(s)");
return -1;
}
if (nonempty_str(file) && nonempty_str(str)) {
-error_report("file and string are mutually exclusive");
+error_setg(errp, "file and string are mutually exclusive");
return -1;
}
if (strlen(name) > FW_CFG_MAX_FILE_PATH - 1) {
-error_report("name too long (max. %d char)", FW_CFG_MAX_FILE_PATH - 1);
+error_setg(errp, "name too long (max. %d char)",
+ FW_CFG_MAX_FILE_PATH - 1);
return -1;
}
if (strncmp(name, "opt/", 4) != 0) {
@@ -2203,7 +2204,7 @@ static int parse_fw_cfg(void *opaque, QemuOpts *opts,
Error **errp)
buf = g_memdup(str, size);
} else {
if (!g_file_get_contents(file, , , NULL)) {
-error_report("can't load %s", file);
+error_setg(errp, "can't load %s", file);
return -1;
}
}
@@ -4429,10 +4430,8 @@ int main(int argc, char **argv, char **envp)
hax_sync_vcpus();
}
-if (qemu_opts_foreach(qemu_find_opts("fw_cfg"),
- parse_fw_cfg, fw_cfg_find(), NULL) != 0) {
-exit(1);
-}
+qemu_opts_foreach(qemu_find_opts("fw_cfg"),
+ parse_fw_cfg, fw_cfg_find(), _fatal);
/* init USB devices */
if (machine_usb(current_machine)) {
--
2.17.1
[Qemu-devel] [PATCH 07/31] 9pfs: Fix CLI parsing crash on error
Calling error_report() in a function that takes an Error ** argument
is suspicious. 9p-handle.c's handle_parse_opts() does that, and then
fails without setting an error. Wrong. Its caller crashes when it
tries to report the error:
$ qemu-system-x86_64 -nodefaults -fsdev id=foo,fsdriver=handle
qemu-system-x86_64: -fsdev id=foo,fsdriver=handle: warning: handle backend
is deprecated
qemu-system-x86_64: -fsdev id=foo,fsdriver=handle: fsdev: No path specified
Segmentation fault (core dumped)
Screwed up when commit 91cda4e8f37 (v2.12.0) converted the function to
Error. Fix by calling error_setg() instead of error_report().
Fixes: 91cda4e8f372602795e3a2f4bd2e3adaf9f82255
Cc: Greg Kurz
Signed-off-by: Markus Armbruster
---
hw/9pfs/9p-handle.c | 6 --
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/hw/9pfs/9p-handle.c b/hw/9pfs/9p-handle.c
index f3641dbe4a..3465b1ef30 100644
--- a/hw/9pfs/9p-handle.c
+++ b/hw/9pfs/9p-handle.c
@@ -19,6 +19,7 @@
#include
#include
#include
+#include "qapi/error.h"
#include "qemu/xattr.h"
#include "qemu/cutils.h"
#include "qemu/error-report.h"
@@ -655,12 +656,13 @@ static int handle_parse_opts(QemuOpts *opts,
FsDriverEntry *fse, Error **errp)
warn_report("handle backend is deprecated");
if (sec_model) {
-error_report("Invalid argument security_model specified with handle
fsdriver");
+error_setg(errp,
+ "Invalid argument security_model specified with handle
fsdriver");
return -1;
}
if (!path) {
-error_report("fsdev: No path specified");
+error_setg(errp, "fsdev: No path specified");
return -1;
}
fse->path = g_strdup(path);
--
2.17.1
