Re: [PATCH 8/8] configure: automatically parse command line for meson -D options
Hi Paolo, as python and meson are required dependencies to building qemu now, can we detecting python/meson at the very begining of configure, even before the --help parameter. On Wed, Jan 13, 2021 at 6:08 AM Paolo Bonzini wrote: > > On 13/01/21 11:31, Daniel P. Berrangé wrote: > >> meson-buildoptions.json | 717 > > I'm not a fan of seeing this file introduced as it has significant > > overlap with meson_options.txt.I feel like the latter has enough > > information present to do an acceptable job for help output. After > > all that's sufficient if we were using meson directly. > > Sorry, I missed this remark. meson-buildoptions.json is not > hand-written. It is the result of Meson's own parsing meson_options.txt > exported as JSON. > > In the commit message "because we parse command-line options before > meson is available, the introspection output is stored in the source > tree. This is the reason for the unattractive diffstat; the number of > JSON lines added is higher than the number of configure lines removed. > Of course the latter are code that must be maintained manually and the > former is not". > > Paolo > > -- 此致 礼 罗勇刚 Yours sincerely, Yonggang Luo
Re: [PATCHv8 2/3] arm-virt: refactor gpios creation
On Wed, Jan 20, 2021 at 12:27:47PM +0300, Maxim Uvarov wrote:
> No functional change. Just refactor code to better
> support secure and normal world gpios.
>
> Signed-off-by: Maxim Uvarov
> ---
> hw/arm/virt.c | 64 ++-
> 1 file changed, 43 insertions(+), 21 deletions(-)
>
> diff --git a/hw/arm/virt.c b/hw/arm/virt.c
> index 96985917d3..c427ce5f81 100644
> --- a/hw/arm/virt.c
> +++ b/hw/arm/virt.c
> @@ -820,17 +820,43 @@ static void virt_powerdown_req(Notifier *n, void
> *opaque)
> }
> }
>
> -static void create_gpio(const VirtMachineState *vms)
> +static void create_gpio_keys(const VirtMachineState *vms,
> + DeviceState *pl061_dev,
> + uint32_t phandle)
> +{
> +gpio_key_dev = sysbus_create_simple("gpio-key", -1,
> +qdev_get_gpio_in(pl061_dev, 3));
> +
> +qemu_fdt_add_subnode(vms->fdt, "/gpio-keys");
> +qemu_fdt_setprop_string(vms->fdt, "/gpio-keys", "compatible",
> "gpio-keys");
> +qemu_fdt_setprop_cell(vms->fdt, "/gpio-keys", "#size-cells", 0);
> +qemu_fdt_setprop_cell(vms->fdt, "/gpio-keys", "#address-cells", 1);
> +
> +qemu_fdt_add_subnode(vms->fdt, "/gpio-keys/poweroff");
> +qemu_fdt_setprop_string(vms->fdt, "/gpio-keys/poweroff",
> +"label", "GPIO Key Poweroff");
> +qemu_fdt_setprop_cell(vms->fdt, "/gpio-keys/poweroff", "linux,code",
> + KEY_POWER);
> +qemu_fdt_setprop_cells(vms->fdt, "/gpio-keys/poweroff",
> + "gpios", phandle, 3, 0);
> +}
> +
> +static void create_gpio_devices(const VirtMachineState *vms, int gpio,
> +MemoryRegion *mem)
> {
> char *nodename;
> DeviceState *pl061_dev;
> -hwaddr base = vms->memmap[VIRT_GPIO].base;
> -hwaddr size = vms->memmap[VIRT_GPIO].size;
> -int irq = vms->irqmap[VIRT_GPIO];
> +hwaddr base = vms->memmap[gpio].base;
> +hwaddr size = vms->memmap[gpio].size;
> +int irq = vms->irqmap[gpio];
> const char compat[] = "arm,pl061\0arm,primecell";
> +SysBusDevice *s;
>
> -pl061_dev = sysbus_create_simple("pl061", base,
> - qdev_get_gpio_in(vms->gic, irq));
> +pl061_dev = qdev_new("pl061");
> +s = SYS_BUS_DEVICE(pl061_dev);
> +sysbus_realize_and_unref(s, &error_fatal);
> +memory_region_add_subregion(mem, base, sysbus_mmio_get_region(s, 0));
> +sysbus_connect_irq(s, 0, qdev_get_gpio_in(vms->gic, irq));
>
> uint32_t phandle = qemu_fdt_alloc_phandle(vms->fdt);
> nodename = g_strdup_printf("/pl061@%" PRIx64, base);
> @@ -847,21 +873,17 @@ static void create_gpio(const VirtMachineState *vms)
> qemu_fdt_setprop_string(vms->fdt, nodename, "clock-names", "apb_pclk");
> qemu_fdt_setprop_cell(vms->fdt, nodename, "phandle", phandle);
>
> -gpio_key_dev = sysbus_create_simple("gpio-key", -1,
> -qdev_get_gpio_in(pl061_dev, 3));
> -qemu_fdt_add_subnode(vms->fdt, "/gpio-keys");
> -qemu_fdt_setprop_string(vms->fdt, "/gpio-keys", "compatible",
> "gpio-keys");
> -qemu_fdt_setprop_cell(vms->fdt, "/gpio-keys", "#size-cells", 0);
> -qemu_fdt_setprop_cell(vms->fdt, "/gpio-keys", "#address-cells", 1);
> -
> -qemu_fdt_add_subnode(vms->fdt, "/gpio-keys/poweroff");
> -qemu_fdt_setprop_string(vms->fdt, "/gpio-keys/poweroff",
> -"label", "GPIO Key Poweroff");
> -qemu_fdt_setprop_cell(vms->fdt, "/gpio-keys/poweroff", "linux,code",
> - KEY_POWER);
> -qemu_fdt_setprop_cells(vms->fdt, "/gpio-keys/poweroff",
> - "gpios", phandle, 3, 0);
> +if (gpio != VIRT_GPIO) {
> +/* Mark as not usable by the normal world */
> +qemu_fdt_setprop_string(vms->fdt, nodename, "status", "disabled");
> +qemu_fdt_setprop_string(vms->fdt, nodename, "secure-status", "okay");
> +}
nit: The above if-block could/should have waited until the next patch to
be added.
> g_free(nodename);
> +
> +/* Child gpio devices */
> +if (gpio == VIRT_GPIO) {
Same nit as above, the next patch is where we should start conditionally
doing stuff based on the gpio type.
> +create_gpio_keys(vms, pl061_dev, phandle);
> +}
> }
>
> static void create_virtio_devices(const VirtMachineState *vms)
> @@ -1990,7 +2012,7 @@ static void machvirt_init(MachineState *machine)
> if (has_ged && aarch64 && firmware_loaded && virt_is_acpi_enabled(vms)) {
> vms->acpi_dev = create_acpi_ged(vms);
> } else {
> -create_gpio(vms);
> +create_gpio_devices(vms, VIRT_GPIO, sysmem);
> }
>
> /* connect powerdown request */
> --
> 2.17.1
>
>
Reviewed-by: Andrew Jones
Thanks,
drew
Re: [RFC PATCH 2/2] gitlab-ci: Add a job building TCI with Clang
On 21/01/2021 21.46, Wainer dos Santos Moschetta wrote: On 1/21/21 3:28 PM, Thomas Huth wrote: On 21/01/2021 19.13, Daniel P. Berrangé wrote: On Thu, Jan 21, 2021 at 03:05:43PM -0300, Wainer dos Santos Moschetta wrote: Hi, On 1/21/21 7:08 AM, Thomas Huth wrote: On 10/01/2021 17.27, Philippe Mathieu-Daudé wrote: Split the current GCC build-tci job in 2, and use Clang compiler in the new job. Signed-off-by: Philippe Mathieu-Daudé --- RFC in case someone have better idea to optimize can respin this patch. .gitlab-ci.yml | 22 -- 1 file changed, 20 insertions(+), 2 deletions(-) I'm not quite sure whether we should go down this road ... if we wanted to have full test coverage for clang, we'd need to duplicate *all* jobs to run them once with gcc and once with clang. And that would be just overkill. I agree with Thomas. I think we already catch most clang-related problems with the clang jobs that we already have in our CI, so problems like the ones that you've tried to address here should be very, very rare. So I'd rather vote for not splitting the job here. We got only one clang job on GitLab CI... build-clang: <<: *native_build_job_definition variables: IMAGE: fedora CONFIGURE_ARGS: --cc=clang --cxx=clang++ TARGETS: alpha-softmmu arm-softmmu m68k-softmmu mips64-softmmu ppc-softmmu s390x-softmmu arm-linux-user MAKE_CHECK_ARGS: check ... and others on Travis: "Clang (user)" "Clang (main-softmmu)" "Clang (other-softmmu)" I guess these three overlap partially with the build-clang job. "[s390x] Clang (disable-tcg)" Don't forget the Cirrus CI jobs for freebsd and macOS will be using CLang too. Right... we should work towards getting cirrus-run into the QEMU-CI, too, to finally have these in the gitlab-ci dashboard, too. So I've some questions: * Can we move those first three Travis jobs to Gitlab CI? (I can work on that) Yeah, if we move those three travis jobs they can replace the existing build-clang job. We don't neccesssarily need to keep them as three separate jobs - that split was just due to the Travis time limits. If a different split works better on GitLab we can do that. Well, if we really want to increase the amount clang jobs, one of them should likely use TCI, as Phillippe suggested. Ok, got it. I won't touch on those jobs. I didn't meant my comment as a "no", but rather as a "needs further discussion first" ... So here's my suggestion: - Keep the current build-tci job as it is - Add a build-clang-user job that compiles with clang and --disable-system - Add a "build-clang-system 2" job that compiles with clang and --enable-tcg-interpreter and uses more or less the same targets as the "build-tci" job. Maybe add the avr-softmmu target here now, too, since it now also has a qtest with TCG (boot-serial-test) - Rename the current "build-clang" job to "build-clang-system 1" and remove the arm-linux-user target and all the targets that are already part of the "build-clang-system 2" / "build-tci" from that job. Then add some other softmmu targets to that job (but take care that it does not exceed the 1h run time limit, so likely not all remaining targets can be added here) - If we feel confident that we got enough test coverage there (together with the clang-based jobs on Cirrus-CI), finally remove the three x86 clang jobs from travis.yml. What do you think? Could you work on such a patch (or patch series), Wainer? Thomas
Re: [PATCHv8 3/3] arm-virt: add secure pl061 for reset/power down
On Wed, Jan 20, 2021 at 12:27:48PM +0300, Maxim Uvarov wrote:
> Add secure pl061 for reset/power down machine from
> the secure world (Arm Trusted Firmware). Connect it
> with gpio-pwr driver.
>
> Signed-off-by: Maxim Uvarov
> ---
> hw/arm/Kconfig| 1 +
> hw/arm/virt.c | 47 +++
> include/hw/arm/virt.h | 2 ++
> 3 files changed, 50 insertions(+)
>
> diff --git a/hw/arm/Kconfig b/hw/arm/Kconfig
> index 0a242e4c5d..13cc42dcc8 100644
> --- a/hw/arm/Kconfig
> +++ b/hw/arm/Kconfig
> @@ -17,6 +17,7 @@ config ARM_VIRT
> select PL011 # UART
> select PL031 # RTC
> select PL061 # GPIO
> +select GPIO_PWR
> select PLATFORM_BUS
> select SMBIOS
> select VIRTIO_MMIO
> diff --git a/hw/arm/virt.c b/hw/arm/virt.c
> index c427ce5f81..060a5f492e 100644
> --- a/hw/arm/virt.c
> +++ b/hw/arm/virt.c
> @@ -153,6 +153,7 @@ static const MemMapEntry base_memmap[] = {
> [VIRT_ACPI_GED] = { 0x0908, ACPI_GED_EVT_SEL_LEN },
> [VIRT_NVDIMM_ACPI] ={ 0x0909, NVDIMM_ACPI_IO_LEN},
> [VIRT_PVTIME] = { 0x090a, 0x0001 },
> +[VIRT_SECURE_GPIO] ={ 0x090b, 0x1000 },
> [VIRT_MMIO] = { 0x0a00, 0x0200 },
> /* ...repeating for a total of NUM_VIRTIO_TRANSPORTS, each of that size
> */
> [VIRT_PLATFORM_BUS] = { 0x0c00, 0x0200 },
> @@ -841,6 +842,43 @@ static void create_gpio_keys(const VirtMachineState *vms,
> "gpios", phandle, 3, 0);
> }
>
> +#define SECURE_GPIO_POWEROFF 0
> +#define SECURE_GPIO_REBOOT 1
> +
> +static void create_gpio_pwr(const VirtMachineState *vms,
This function is specific to the secure view. I think it should have
"secure" in its name.
> +DeviceState *pl061_dev,
> +uint32_t phandle)
> +{
> +DeviceState *gpio_pwr_dev;
> +
> +/* gpio-pwr */
> +gpio_pwr_dev = sysbus_create_simple("gpio-pwr", -1, NULL);
Should this device be in secure memory?
> +
> +/* connect secure pl061 to gpio-pwr */
> +qdev_connect_gpio_out(pl061_dev, SECURE_GPIO_REBOOT,
> + qdev_get_gpio_in_named(gpio_pwr_dev, "reset", 0));
> +qdev_connect_gpio_out(pl061_dev, SECURE_GPIO_POWEROFF,
> + qdev_get_gpio_in_named(gpio_pwr_dev, "shutdown",
> 0));
> +
> +qemu_fdt_add_subnode(vms->fdt, "/gpio-poweroff");
> +qemu_fdt_setprop_string(vms->fdt, "/gpio-poweroff", "compatible",
> +"gpio-poweroff");
> +qemu_fdt_setprop_cells(vms->fdt, "/gpio-poweroff",
> + "gpios", phandle, SECURE_GPIO_POWEROFF, 0);
> +qemu_fdt_setprop_string(vms->fdt, "/gpio-poweroff", "status",
> "disabled");
> +qemu_fdt_setprop_string(vms->fdt, "/gpio-poweroff", "secure-status",
> +"okay");
> +
> +qemu_fdt_add_subnode(vms->fdt, "/gpio-restart");
> +qemu_fdt_setprop_string(vms->fdt, "/gpio-restart", "compatible",
> +"gpio-restart");
> +qemu_fdt_setprop_cells(vms->fdt, "/gpio-restart",
> + "gpios", phandle, SECURE_GPIO_REBOOT, 0);
> +qemu_fdt_setprop_string(vms->fdt, "/gpio-restart", "status", "disabled");
> +qemu_fdt_setprop_string(vms->fdt, "/gpio-restart", "secure-status",
> +"okay");
> +}
> +
> static void create_gpio_devices(const VirtMachineState *vms, int gpio,
> MemoryRegion *mem)
> {
> @@ -883,6 +921,8 @@ static void create_gpio_devices(const VirtMachineState
> *vms, int gpio,
> /* Child gpio devices */
> if (gpio == VIRT_GPIO) {
> create_gpio_keys(vms, pl061_dev, phandle);
> +} else {
> +create_gpio_pwr(vms, pl061_dev, phandle);
> }
> }
>
> @@ -2015,6 +2055,10 @@ static void machvirt_init(MachineState *machine)
> create_gpio_devices(vms, VIRT_GPIO, sysmem);
> }
>
> +if (vms->secure && !vmc->no_secure_gpio) {
> +create_gpio_devices(vms, VIRT_SECURE_GPIO, secure_sysmem);
> +}
> +
> /* connect powerdown request */
> vms->powerdown_notifier.notify = virt_powerdown_req;
> qemu_register_powerdown_notifier(&vms->powerdown_notifier);
> @@ -2630,8 +2674,11 @@ DEFINE_VIRT_MACHINE_AS_LATEST(6, 0)
>
> static void virt_machine_5_2_options(MachineClass *mc)
> {
> +VirtMachineClass *vmc = VIRT_MACHINE_CLASS(OBJECT_CLASS(mc));
> +
> virt_machine_6_0_options(mc);
> compat_props_add(mc->compat_props, hw_compat_5_2, hw_compat_5_2_len);
> +vmc->no_secure_gpio = true;
> }
> DEFINE_VIRT_MACHINE(5, 2)
>
> diff --git a/include/hw/arm/virt.h b/include/hw/arm/virt.h
> index abf54fab49..6f6c85ffcf 100644
> --- a/include/hw/arm/virt.h
> +++ b/include/hw/arm/virt.h
> @@ -81,6 +81,7 @@ enum {
> VIRT_GPIO,
> VIRT_SECURE_UART,
> VIRT_SECURE_MEM,
> +VIRT_SECURE_GPIO,
> VIRT
Re: [PATCH 04/25] keyval: accept escaped commas in implied option
Paolo Bonzini writes:
[...]
> diff --git a/util/keyval.c b/util/keyval.c
> index be34928813..eb9b9c55ec 100644
> --- a/util/keyval.c
> +++ b/util/keyval.c
> @@ -16,8 +16,8 @@
> * key-vals = [ key-val { ',' key-val } [ ',' ] ]
> * key-val = key '=' val | help
> * key = key-fragment { '.' key-fragment }
> - * key-fragment = / [^=,.]+ /
> - * val = { / [^,]+ / | ',,' }
> + * key-fragment = { / [^=,.] / | ',,' }
> + * val = { / [^,] / | ',,' }
> * help = 'help' | '?'
> *
> * Semantics defined by reduction to JSON:
> @@ -78,13 +78,13 @@
> * Alternative syntax for use with an implied key:
> *
> * key-vals = [ key-val-1st { ',' key-val } [ ',' ] ]
> - * key-val-1st = val-no-key | key-val
> - * val-no-key = / [^=,]+ / - help
> + * key-val-1st = (val-no-key - help) | key-val
> + * val-no-key = { / [^=,] / | ',,' }
I finally remembered why I made val-no-key non-empty: to avoid amiguity.
Before the patch, "" can only be parsed as empty key-vals. Results in
an empty QDict.
Afterwards, the grammar is ambiguous: "" can also be parsed as empty
val-no-key, reduced via key-val-1st to non-empty key-vals. Results in a
QDict with one entry mapping the implied key to "".
I'm a bit concerned I similarly forgot something that made me avoid ',,'
escapes in val-no-key.
Even if we brushed off the ambiguous grammar issue (and we should not!),
desugaring "" into "implied=" feels unwise, and ",k=v" into
"implied=,k=v" only slightly less so.
Let's keep val-no-key non-empty.
Ripple effect... I made val-no-key match key (almost):
val-no-key = / [^=,]+ /
key = key-fragment { '.' key-fragment }
key-fragment = / [^=,.]+ /
The only difference is val-no-key accepts consecutive '.'.
Commit 8bf12c4f75 "keyval: Parse help options" muddied the waters a bit
by adding '- help' to val-no-key.
Your commit moves it to key-val-1st (good). It also permits empty
key-fragment, and thus consecutive '.' (good because it makes val-no-key
match key exactly, but also possibly confusing because key-fragment
can't actually be empty due to the "Key-fragments must be valid QAPI
names or consist only of decimal digits" condition). Okay.
It also changed both val-no-key and key to accept empty. We need to
keep *both* non-empty.
Your change to val is not wrong, but I prefer the old version, because
it's closer to how the code works.
> *
> * where val-no-key is syntactic sugar for implied-key=val-no-key.
> *
> - * Note that you can't use the sugared form when the value contains
> - * '=' or ','.
> + * Note that you can't use the sugared form when the value is empty
You can with your grammar change, unless the code doesn't match the
grammar. Which would be a bug.
> + * or contains '='.
> */
[...]
I apologize for sitting on this patch for so long. Something felt
wrong, but I couldn't put a finger on it. Now I can at least for the
empty val-no-key part.
Re: [PULL 10/11] vnc: move initialization to framebuffer_update_request
> This patch breaks QEMU for me.
> The symptom is the following: in virt-manager, the display remains dead
> (black), when I start an OVMF guest. At the same time, unusually high
> CPU load can be seen on the host; it makes me think that virt-manager is
> trying, in a busy loop, to complete the VNC handshake, or some such.
> Ultimately, although the guest boots up fine, virt-manager gives up, and
> the display window says "Viewer was disconnected".
It is the vnc_colordepth() call. Seems gtk-vnc sends a update request
with incremental=0 as response to the VNC_ENCODING_WMVi message. So
sending that as response to an incremental=0 update request creates an
endless loop ...
take care,
Gerd
diff --git a/ui/vnc.c b/ui/vnc.c
index d429bfee5a65..0a3e2f4aa98c 100644
--- a/ui/vnc.c
+++ b/ui/vnc.c
@@ -2041,7 +2041,6 @@ static void framebuffer_update_request(VncState *vs, int
incremental,
} else {
vs->update = VNC_STATE_UPDATE_FORCE;
vnc_set_area_dirty(vs->dirty, vs->vd, x, y, w, h);
-vnc_colordepth(vs);
vnc_desktop_resize(vs);
vnc_led_state_change(vs);
vnc_cursor_define(vs);
Re: Thread safety of coroutine-sigaltstack
On 20.01.21 18:25, Laszlo Ersek wrote: [...] A simple grep for SIGUSR2 seems to indicate that SIGUSR2 is not used by system emulation for anything else, in practice. Is it possible to dedicate SIGUSR2 explicitly to coroutine-sigaltstack, and set up the action beforehand, from some init function that executes on a "central" thread, before qemu_coroutine_new() is ever called? I wrote a patch to that effect, but just before sending I wondered whether SIGUSR2 cannot be registered by the “guest” in user-mode emulation, and whether that would then break coroutines from there on. (I have no experience dealing with user-mode emulation, but it does look like the guest can just register handlers for any signal but SIGSEGV and SIGBUS.) Max
[PATCH] vnc: drop vnc_colordepth() call.
With gtk-vnc (which supports VNC_FEATURE_WMVI) this results in
sending a VNC_ENCODING_WMVi message to the client. Which in turn
seems to make gtk-vnc respond with another non-incremental update
request. Hello endless loop ...
Drop the call.
Fixes: 9e1632ad07ca ("vnc: move initialization to framebuffer_update_request")
Reported-by: Laszlo Ersek
Signed-off-by: Gerd Hoffmann
---
ui/vnc.c | 1 -
1 file changed, 1 deletion(-)
diff --git a/ui/vnc.c b/ui/vnc.c
index d429bfee5a65..0a3e2f4aa98c 100644
--- a/ui/vnc.c
+++ b/ui/vnc.c
@@ -2041,7 +2041,6 @@ static void framebuffer_update_request(VncState *vs, int
incremental,
} else {
vs->update = VNC_STATE_UPDATE_FORCE;
vnc_set_area_dirty(vs->dirty, vs->vd, x, y, w, h);
-vnc_colordepth(vs);
vnc_desktop_resize(vs);
vnc_led_state_change(vs);
vnc_cursor_define(vs);
--
2.29.2
[PATCH 0/2] meson: Try to clarify TCG / TCI options for new users
Some new users get confused between 'TCG' and 'TCI' and enable TCI when TCG is better for they needs. Try to clarify it is better to not use TCI when native backend is available. Note, before Meson, warnings were summarized at the end of ./configure. Now they are displayed earlier, and likely missed IMHO. No clue how to improve that :/ Based-on: <20210121095616.1471869-1-phi...@redhat.com> Philippe Mathieu-Daudé (2): meson: Explicit TCG backend used meson: Warn when TCI is selected but TCG backend is available meson.build | 9 - 1 file changed, 8 insertions(+), 1 deletion(-) -- 2.26.2
[PATCH 2/2] meson: Warn when TCI is selected but TCG backend is available
Some new users get confused with 'TCG' and 'TCI', and enable TCI
support expecting to enable TCG.
Emit a warning when native TCG backend is available on the
host architecture, mentioning this is a suboptimal configuration.
Signed-off-by: Philippe Mathieu-Daudé
---
meson.build | 3 +++
1 file changed, 3 insertions(+)
diff --git a/meson.build b/meson.build
index 0a645e54662..7aa52d306c6 100644
--- a/meson.build
+++ b/meson.build
@@ -234,6 +234,9 @@
error('Unsupported CPU @0@, try --enable-tcg-interpreter'.format(cpu))
endif
endif
+ if 'CONFIG_TCG_INTERPRETER' in config_host and cpu in supported_cpus
+warning('Experimental TCI requested while native TCG is available on @0@,
suboptimal performance expected'.format(cpu))
+ endif
accelerators += 'CONFIG_TCG'
config_host += { 'CONFIG_TCG': 'y' }
endif
--
2.26.2
[PATCH 1/2] meson: Explicit TCG backend used
Signed-off-by: Philippe Mathieu-Daudé
---
meson.build | 6 +-
1 file changed, 5 insertions(+), 1 deletion(-)
diff --git a/meson.build b/meson.build
index 8535a83fb70..0a645e54662 100644
--- a/meson.build
+++ b/meson.build
@@ -2419,8 +2419,12 @@
endif
summary_info += {'TCG support': config_all.has_key('CONFIG_TCG')}
if config_all.has_key('CONFIG_TCG')
+ if config_host.has_key('CONFIG_TCG_INTERPRETER')
+summary_info += {'TCG backend': 'TCG interpreter (experimental)'}
+ else
+summary_info += {'TCG backend': 'native (@0@)'.format(cpu)}
+ endif
summary_info += {'TCG debug enabled':
config_host.has_key('CONFIG_DEBUG_TCG')}
- summary_info += {'TCG interpreter':
config_host.has_key('CONFIG_TCG_INTERPRETER')}
endif
summary_info += {'target list': ' '.join(target_dirs)}
if have_system
--
2.26.2
Re: [PATCH] linux-user/mmap: Avoid asserts for out of range mremap calls
On Fri, 2021-01-08 at 17:42 +, Richard Purdie wrote:
> If mremap() is called without the MREMAP_MAYMOVE flag with a start address
> just before the end of memory (reserved_va) where new_size would exceed
> it (and GUEST_ADDR_MAX), the assert(end - 1 <= GUEST_ADDR_MAX) in
> page_set_flags() would trigger.
>
> Add an extra guard to the guest_range_valid() checks to prevent this and
> avoid asserting binaries when reserved_va is set.
>
> This meant a bug I was seeing locally now gives the same behaviour
> regardless of whether reserved_va is set or not.
>
> Signed-off-by: Richard Purdie
> Index: qemu-5.2.0/linux-user/mmap.c
> ===
> --- qemu-5.2.0.orig/linux-user/mmap.c
> +++ qemu-5.2.0/linux-user/mmap.c
> @@ -727,7 +727,9 @@ abi_long target_mremap(abi_ulong old_add
>
>
> if (!guest_range_valid(old_addr, old_size) ||
> ((flags & MREMAP_FIXED) &&
> - !guest_range_valid(new_addr, new_size))) {
> + !guest_range_valid(new_addr, new_size)) ||
> +((flags & MREMAP_MAYMOVE) == 0 &&
> + !guest_range_valid(old_addr, new_size))) {
> errno = ENOMEM;
> return -1;
> }
>
>
Any comments on this? I believe its a valid issue that needs fixing and
multiple distros appear to be carrying fixes in this area related to
this.
Cheers,
Richard
Re: [RFC PATCH] linux-user/mmap: Return EFAULT for invalid addresses
On Fri, 2021-01-08 at 17:46 +, Richard Purdie wrote:
> When using qemu-i386 to run gobject introspection parts of a webkitgtk
> build using musl as libc on a 64 bit host, it sits in an infinite loop
> of mremap calls of ever decreasing/increasing addresses.
>
> I suspect something in the musl memory allocation code loops indefinitely
> if it only sees ENOMEM and only exits when it hits EFAULT.
>
> According to the docs, trying to mremap outside the address space
> can/should return EFAULT and changing this allows the build to succeed.
>
> There was previous discussion of this as it used to work before qemu 2.11
> and we've carried hacks to work around it since, this appears to be a
> better fix of the real issue?
>
> Signed-off-by: Richard Purdie
> Index: qemu-5.2.0/linux-user/mmap.c
> ===
> --- qemu-5.2.0.orig/linux-user/mmap.c
> +++ qemu-5.2.0/linux-user/mmap.c
> @@ -727,7 +727,7 @@ abi_long target_mremap(abi_ulong old_add
> !guest_range_valid(new_addr, new_size)) ||
> ((flags & MREMAP_MAYMOVE) == 0 &&
> !guest_range_valid(old_addr, new_size))) {
> -errno = ENOMEM;
> +errno = EFAULT;
> return -1;
> }
Any comments on this? I believe its a valid issue that needs fixing and
multiple distros appear to be carrying fixes in this area related to
this.
Cheers,
Richard
Re: [PATCH 1/2] meson: Explicit TCG backend used
On 22/01/2021 10.22, Philippe Mathieu-Daudé wrote:
Signed-off-by: Philippe Mathieu-Daudé
---
meson.build | 6 +-
1 file changed, 5 insertions(+), 1 deletion(-)
diff --git a/meson.build b/meson.build
index 8535a83fb70..0a645e54662 100644
--- a/meson.build
+++ b/meson.build
@@ -2419,8 +2419,12 @@
endif
summary_info += {'TCG support': config_all.has_key('CONFIG_TCG')}
if config_all.has_key('CONFIG_TCG')
+ if config_host.has_key('CONFIG_TCG_INTERPRETER')
+summary_info += {'TCG backend': 'TCG interpreter (experimental)'}
maybe say "experimental and slow" in the parentheses?
+ else
+summary_info += {'TCG backend': 'native (@0@)'.format(cpu)}
+ endif
summary_info += {'TCG debug enabled':
config_host.has_key('CONFIG_DEBUG_TCG')}
- summary_info += {'TCG interpreter':
config_host.has_key('CONFIG_TCG_INTERPRETER')}
endif
summary_info += {'target list': ' '.join(target_dirs)}
if have_system
Reviewed-by: Thomas Huth
Re: [PATCH 2/2] meson: Warn when TCI is selected but TCG backend is available
On 22/01/2021 10.22, Philippe Mathieu-Daudé wrote:
Some new users get confused with 'TCG' and 'TCI', and enable TCI
support expecting to enable TCG.
Emit a warning when native TCG backend is available on the
host architecture, mentioning this is a suboptimal configuration.
Signed-off-by: Philippe Mathieu-Daudé
---
meson.build | 3 +++
1 file changed, 3 insertions(+)
diff --git a/meson.build b/meson.build
index 0a645e54662..7aa52d306c6 100644
--- a/meson.build
+++ b/meson.build
@@ -234,6 +234,9 @@
error('Unsupported CPU @0@, try --enable-tcg-interpreter'.format(cpu))
endif
endif
+ if 'CONFIG_TCG_INTERPRETER' in config_host and cpu in supported_cpus
+warning('Experimental TCI requested while native TCG is available on @0@,
suboptimal performance expected'.format(cpu))
+ endif
accelerators += 'CONFIG_TCG'
config_host += { 'CONFIG_TCG': 'y' }
endif
Reviewed-by: Thomas Huth
... maybe you could also add some wording to the help text of the configure
script? Or maybe we could simply rename the "--enable-tcg-interpreter"
option into "--enable-tci" to avoid confusion with the normal TCG ?
Re: [PATCH 2/2] meson: Warn when TCI is selected but TCG backend is available
On Fri, Jan 22, 2021 at 10:43 AM Thomas Huth wrote:
> On 22/01/2021 10.22, Philippe Mathieu-Daudé wrote:
> > Some new users get confused with 'TCG' and 'TCI', and enable TCI
> > support expecting to enable TCG.
> >
> > Emit a warning when native TCG backend is available on the
> > host architecture, mentioning this is a suboptimal configuration.
> >
> > Signed-off-by: Philippe Mathieu-Daudé
> > ---
> > meson.build | 3 +++
> > 1 file changed, 3 insertions(+)
> >
> > diff --git a/meson.build b/meson.build
> > index 0a645e54662..7aa52d306c6 100644
> > --- a/meson.build
> > +++ b/meson.build
> > @@ -234,6 +234,9 @@
> > error('Unsupported CPU @0@, try
> > --enable-tcg-interpreter'.format(cpu))
> > endif
> > endif
> > + if 'CONFIG_TCG_INTERPRETER' in config_host and cpu in supported_cpus
> > +warning('Experimental TCI requested while native TCG is available on
> > @0@, suboptimal performance expected'.format(cpu))
> > + endif
> > accelerators += 'CONFIG_TCG'
> > config_host += { 'CONFIG_TCG': 'y' }
> > endif
> >
>
> Reviewed-by: Thomas Huth
>
> ... maybe you could also add some wording to the help text of the configure
> script? Or maybe we could simply rename the "--enable-tcg-interpreter"
> option into "--enable-tci" to avoid confusion with the normal TCG ?
I also thought about renaming as --enable-experimental-tci but then doubt that
would help, some users just want to enable everything :)
Re: [PATCH 2/2] meson: Warn when TCI is selected but TCG backend is available
On 22/01/2021 10.46, Philippe Mathieu-Daudé wrote:
On Fri, Jan 22, 2021 at 10:43 AM Thomas Huth wrote:
On 22/01/2021 10.22, Philippe Mathieu-Daudé wrote:
Some new users get confused with 'TCG' and 'TCI', and enable TCI
support expecting to enable TCG.
Emit a warning when native TCG backend is available on the
host architecture, mentioning this is a suboptimal configuration.
Signed-off-by: Philippe Mathieu-Daudé
---
meson.build | 3 +++
1 file changed, 3 insertions(+)
diff --git a/meson.build b/meson.build
index 0a645e54662..7aa52d306c6 100644
--- a/meson.build
+++ b/meson.build
@@ -234,6 +234,9 @@
error('Unsupported CPU @0@, try --enable-tcg-interpreter'.format(cpu))
endif
endif
+ if 'CONFIG_TCG_INTERPRETER' in config_host and cpu in supported_cpus
+warning('Experimental TCI requested while native TCG is available on @0@,
suboptimal performance expected'.format(cpu))
+ endif
accelerators += 'CONFIG_TCG'
config_host += { 'CONFIG_TCG': 'y' }
endif
Reviewed-by: Thomas Huth
... maybe you could also add some wording to the help text of the configure
script? Or maybe we could simply rename the "--enable-tcg-interpreter"
option into "--enable-tci" to avoid confusion with the normal TCG ?
I also thought about renaming as --enable-experimental-tci but then doubt that
would help, some users just want to enable everything :)
Then we should rename it into --disable-native-tcg ;-)
Thomas
Re: [PATCH 2/2] meson: Warn when TCI is selected but TCG backend is available
Am 22.01.21 um 10:47 schrieb Thomas Huth:
On 22/01/2021 10.46, Philippe Mathieu-Daudé wrote:
On Fri, Jan 22, 2021 at 10:43 AM Thomas Huth wrote:
On 22/01/2021 10.22, Philippe Mathieu-Daudé wrote:
Some new users get confused with 'TCG' and 'TCI', and enable TCI
support expecting to enable TCG.
Emit a warning when native TCG backend is available on the
host architecture, mentioning this is a suboptimal configuration.
Signed-off-by: Philippe Mathieu-Daudé
---
meson.build | 3 +++
1 file changed, 3 insertions(+)
diff --git a/meson.build b/meson.build
index 0a645e54662..7aa52d306c6 100644
--- a/meson.build
+++ b/meson.build
@@ -234,6 +234,9 @@
error('Unsupported CPU @0@, try
--enable-tcg-interpreter'.format(cpu))
endif
endif
+ if 'CONFIG_TCG_INTERPRETER' in config_host and cpu in
supported_cpus
+ warning('Experimental TCI requested while native TCG is
available on @0@, suboptimal performance expected'.format(cpu))
+ endif
accelerators += 'CONFIG_TCG'
config_host += { 'CONFIG_TCG': 'y' }
endif
Reviewed-by: Thomas Huth
... maybe you could also add some wording to the help text of the
configure
script? Or maybe we could simply rename the "--enable-tcg-interpreter"
option into "--enable-tci" to avoid confusion with the normal TCG ?
I also thought about renaming as --enable-experimental-tci but then
doubt that
would help, some users just want to enable everything :)
Then we should rename it into --disable-native-tcg ;-)
Thomas
Both patches are fine (also optionally with the suggested addition of
"slow"), so
Reviewed-by: Stefan Weil
I think that --enable-tci would increase the TCI/TCG confusion and
suggest to keep the current --enable-tcg-interpreter as most experts
know that an interpreter is usually something which is slow.
As soon as time permits I also plan to make a new effort to integrate
TCI as a run time option. Some years ago it was not possible to have
code which supports both native and interpreted TCG, but this might have
changed now. If it is possible, this would simplify CI a lot, and users
could select the interpreter via a command line argument.
Stefan
[PATCH] util/log: flush TB cache when log level changes
Sometimes we need to collect the translation logs starting
from some point of the execution. Some TB listings may
be missed in this case, when blocks were translated before.
This patch clears TB cache to allow re-translation of such
code blocks.
Signed-off-by: Pavel Dovgalyuk
---
accel/tcg/translate-all.c |8
include/sysemu/tcg.h |1 +
stubs/meson.build |1 +
stubs/tcg.c | 12
util/log.c|3 +++
5 files changed, 25 insertions(+)
create mode 100644 stubs/tcg.c
diff --git a/accel/tcg/translate-all.c b/accel/tcg/translate-all.c
index e9de6ff9dd..3acb227c57 100644
--- a/accel/tcg/translate-all.c
+++ b/accel/tcg/translate-all.c
@@ -1461,6 +1461,14 @@ void tb_flush(CPUState *cpu)
}
}
+void tb_flush_all(void)
+{
+CPUState *cpu;
+CPU_FOREACH(cpu) {
+tb_flush(cpu);
+}
+}
+
/*
* Formerly ifdef DEBUG_TB_CHECK. These debug functions are user-mode-only,
* so in order to prevent bit rot we compile them unconditionally in user-mode,
diff --git a/include/sysemu/tcg.h b/include/sysemu/tcg.h
index 00349fb18a..7415f11022 100644
--- a/include/sysemu/tcg.h
+++ b/include/sysemu/tcg.h
@@ -9,6 +9,7 @@
#define SYSEMU_TCG_H
void tcg_exec_init(unsigned long tb_size, int splitwx);
+void tb_flush_all(void);
#ifdef CONFIG_TCG
extern bool tcg_allowed;
diff --git a/stubs/meson.build b/stubs/meson.build
index 80b1d81a31..95e70f8542 100644
--- a/stubs/meson.build
+++ b/stubs/meson.build
@@ -38,6 +38,7 @@ stub_ss.add(files('set-fd-handler.c'))
stub_ss.add(files('sysbus.c'))
stub_ss.add(files('target-get-monitor-def.c'))
stub_ss.add(files('target-monitor-defs.c'))
+stub_ss.add(files('tcg.c'))
stub_ss.add(files('tpm.c'))
stub_ss.add(files('trace-control.c'))
stub_ss.add(files('uuid.c'))
diff --git a/stubs/tcg.c b/stubs/tcg.c
new file mode 100644
index 00..775a748c77
--- /dev/null
+++ b/stubs/tcg.c
@@ -0,0 +1,12 @@
+/*
+ * TCG stubs
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2 or later.
+ * See the COPYING file in the top-level directory.
+ */
+
+#include "sysemu/tcg.h"
+
+void tb_flush_all(void)
+{
+}
diff --git a/util/log.c b/util/log.c
index 2ee1500bee..2ff342a91b 100644
--- a/util/log.c
+++ b/util/log.c
@@ -26,6 +26,7 @@
#include "trace/control.h"
#include "qemu/thread.h"
#include "qemu/lockable.h"
+#include "sysemu/tcg.h"
static char *logfilename;
static QemuMutex qemu_logfile_mutex;
@@ -84,6 +85,8 @@ void qemu_set_log(int log_flags)
#ifdef CONFIG_TRACE_LOG
qemu_loglevel |= LOG_TRACE;
#endif
+tb_flush_all();
+
/*
* In all cases we only log if qemu_loglevel is set.
* Also:
[PATCH] gitlab-ci.yml: Use the whole tree as artifacts to speed up the CI
Currently, our check-system-* jobs are recompiling the whole sources
again. This happens due to the fact that the jobs are checking out
the whole source tree and required submodules again, and only try
to use the "build" directory with the binaries and object files
as an artifact from the previous stage - which simply does not work
anymore (with the current version of meson). Due to some changed
time stamps, meson is always trying to rebuild the whole tree.
So instead of trying to marry a freshly checked out source tree
with the pre-built binaries in these jobs, let's simply pass the
whole source including the submodules and the build tree as artifact
to the test jobs. That way timestamps get preserved and there is
no rebuild of the sources anymore. This saves ca. 15 - 20 minutes
of precious CI cycles in each run.
Signed-off-by: Thomas Huth
---
This is how a job looked like before my patch, running for 42 minutes:
https://gitlab.com/huth/qemu/-/jobs/978432757
And this is how it looks like afterwards - it just took 18 minutes:
https://gitlab.com/huth/qemu/-/jobs/979500316
.gitlab-ci.d/containers.yml | 1 +
.gitlab-ci.yml | 40 +
2 files changed, 28 insertions(+), 13 deletions(-)
diff --git a/.gitlab-ci.yml b/.gitlab-ci.yml
index e2f9c99e27..d55280661f 100644
--- a/.gitlab-ci.yml
+++ b/.gitlab-ci.yml
@@ -39,7 +39,6 @@ include:
image: $CI_REGISTRY_IMAGE/qemu/$IMAGE:latest
script:
- cd build
-- find . -type f -exec touch {} +
- make $MAKE_CHECK_ARGS
.acceptance_template: &acceptance_definition
@@ -83,8 +82,7 @@ build-system-alpine:
artifacts:
expire_in: 2 days
paths:
- - .git-submodule-status
- - build
+ - "*"
check-system-alpine:
<<: *native_test_job_definition
@@ -92,6 +90,7 @@ check-system-alpine:
- job: build-system-alpine
artifacts: true
variables:
+GIT_CHECKOUT: "false"
IMAGE: alpine
MAKE_CHECK_ARGS: check
@@ -101,6 +100,7 @@ acceptance-system-alpine:
- job: build-system-alpine
artifacts: true
variables:
+GIT_CHECKOUT: "false"
IMAGE: alpine
MAKE_CHECK_ARGS: check-acceptance
<<: *acceptance_definition
@@ -116,7 +116,7 @@ build-system-ubuntu:
artifacts:
expire_in: 2 days
paths:
- - build
+ - "*"
check-system-ubuntu:
<<: *native_test_job_definition
@@ -124,6 +124,7 @@ check-system-ubuntu:
- job: build-system-ubuntu
artifacts: true
variables:
+GIT_CHECKOUT: "false"
IMAGE: ubuntu2004
MAKE_CHECK_ARGS: check
@@ -133,6 +134,7 @@ acceptance-system-ubuntu:
- job: build-system-ubuntu
artifacts: true
variables:
+GIT_CHECKOUT: "false"
IMAGE: ubuntu2004
MAKE_CHECK_ARGS: check-acceptance
<<: *acceptance_definition
@@ -148,7 +150,7 @@ build-system-debian:
artifacts:
expire_in: 2 days
paths:
- - build
+ - "*"
check-system-debian:
<<: *native_test_job_definition
@@ -156,6 +158,7 @@ check-system-debian:
- job: build-system-debian
artifacts: true
variables:
+GIT_CHECKOUT: "false"
IMAGE: debian-amd64
MAKE_CHECK_ARGS: check
@@ -170,7 +173,7 @@ build-tools-and-docs-debian:
artifacts:
expire_in: 2 days
paths:
- - build
+ - "*"
acceptance-system-debian:
<<: *native_test_job_definition
@@ -178,6 +181,7 @@ acceptance-system-debian:
- job: build-system-debian
artifacts: true
variables:
+GIT_CHECKOUT: "false"
IMAGE: debian-amd64
MAKE_CHECK_ARGS: check-acceptance
<<: *acceptance_definition
@@ -194,7 +198,7 @@ build-system-fedora:
artifacts:
expire_in: 2 days
paths:
- - build
+ - "*"
check-system-fedora:
<<: *native_test_job_definition
@@ -202,6 +206,7 @@ check-system-fedora:
- job: build-system-fedora
artifacts: true
variables:
+GIT_CHECKOUT: "false"
IMAGE: fedora
MAKE_CHECK_ARGS: check
@@ -211,6 +216,7 @@ acceptance-system-fedora:
- job: build-system-fedora
artifacts: true
variables:
+GIT_CHECKOUT: "false"
IMAGE: fedora
MAKE_CHECK_ARGS: check-acceptance
<<: *acceptance_definition
@@ -226,7 +232,7 @@ build-system-centos:
artifacts:
expire_in: 2 days
paths:
- - build
+ - "*"
check-system-centos:
<<: *native_test_job_definition
@@ -234,6 +240,7 @@ check-system-centos:
- job: build-system-centos
artifacts: true
variables:
+GIT_CHECKOUT: "false"
IMAGE: centos8
MAKE_CHECK_ARGS: check
@@ -243,6 +250,7 @@ acceptance-system-centos:
- job: build-system-centos
artifacts: true
variables:
+GIT_CHECKOUT: "false"
IMAGE: centos8
MAKE_CHECK_ARGS: check-acceptance
<<: *acceptance_definition
@@ -257,7 +265,7 @@ build-system-opensuse:
artifacts:
expire_in: 2 days
paths:
- - build
+ - "*"
check-system-opensuse:
<<: *native_test_job_definition
@@ -265,6 +273,
Re: [PATCHv8 3/3] arm-virt: add secure pl061 for reset/power down
On Fri, 22 Jan 2021 at 08:29, Andrew Jones wrote:
>
> On Wed, Jan 20, 2021 at 12:27:48PM +0300, Maxim Uvarov wrote:
> > Add secure pl061 for reset/power down machine from
> > the secure world (Arm Trusted Firmware). Connect it
> > with gpio-pwr driver.
> >
> > Signed-off-by: Maxim Uvarov
> > ---
> > hw/arm/Kconfig| 1 +
> > hw/arm/virt.c | 47 +++
> > include/hw/arm/virt.h | 2 ++
> > 3 files changed, 50 insertions(+)
> >
> > diff --git a/hw/arm/Kconfig b/hw/arm/Kconfig
> > index 0a242e4c5d..13cc42dcc8 100644
> > --- a/hw/arm/Kconfig
> > +++ b/hw/arm/Kconfig
> > @@ -17,6 +17,7 @@ config ARM_VIRT
> > select PL011 # UART
> > select PL031 # RTC
> > select PL061 # GPIO
> > +select GPIO_PWR
> > select PLATFORM_BUS
> > select SMBIOS
> > select VIRTIO_MMIO
> > diff --git a/hw/arm/virt.c b/hw/arm/virt.c
> > index c427ce5f81..060a5f492e 100644
> > --- a/hw/arm/virt.c
> > +++ b/hw/arm/virt.c
> > @@ -153,6 +153,7 @@ static const MemMapEntry base_memmap[] = {
> > [VIRT_ACPI_GED] = { 0x0908, ACPI_GED_EVT_SEL_LEN },
> > [VIRT_NVDIMM_ACPI] ={ 0x0909, NVDIMM_ACPI_IO_LEN},
> > [VIRT_PVTIME] = { 0x090a, 0x0001 },
> > +[VIRT_SECURE_GPIO] ={ 0x090b, 0x1000 },
> > [VIRT_MMIO] = { 0x0a00, 0x0200 },
> > /* ...repeating for a total of NUM_VIRTIO_TRANSPORTS, each of that
> > size */
> > [VIRT_PLATFORM_BUS] = { 0x0c00, 0x0200 },
> > @@ -841,6 +842,43 @@ static void create_gpio_keys(const VirtMachineState
> > *vms,
> > "gpios", phandle, 3, 0);
> > }
> >
> > +#define SECURE_GPIO_POWEROFF 0
> > +#define SECURE_GPIO_REBOOT 1
> > +
> > +static void create_gpio_pwr(const VirtMachineState *vms,
>
> This function is specific to the secure view. I think it should have
> "secure" in its name.
>
> > +DeviceState *pl061_dev,
> > +uint32_t phandle)
> > +{
> > +DeviceState *gpio_pwr_dev;
> > +
> > +/* gpio-pwr */
> > +gpio_pwr_dev = sysbus_create_simple("gpio-pwr", -1, NULL);
>
> Should this device be in secure memory?
It's not in any memory at all -- -1 as the address argument
to sysbus_create_simple() means "no MMIO regions to map". The
only way it's connected to the rest of the system is via the
secure-only PL061, so the NS world can't get at it.
(sysbus_create_simple("device", -1, NULL) is equivalent to:
dev = qdev_new("device");
sysbus_realize_and_unref(SYSBUS_DEVICE(dev), &error_fatal);
)
thanks
-- PMM
Re: Thread safety of coroutine-sigaltstack
On Fri, 22 Jan 2021 at 08:50, Max Reitz wrote: > > On 20.01.21 18:25, Laszlo Ersek wrote: > > [...] > > > A simple grep for SIGUSR2 seems to indicate that SIGUSR2 is not used by > > system emulation for anything else, in practice. Is it possible to > > dedicate SIGUSR2 explicitly to coroutine-sigaltstack, and set up the > > action beforehand, from some init function that executes on a "central" > > thread, before qemu_coroutine_new() is ever called? > > I wrote a patch to that effect, but just before sending I wondered > whether SIGUSR2 cannot be registered by the “guest” in user-mode > emulation, and whether that would then break coroutines from there on. > > (I have no experience dealing with user-mode emulation, but it does look > like the guest can just register handlers for any signal but SIGSEGV and > SIGBUS.) Yes, SIGUSR2 is for the guest in user-emulation mode. OTOH do we even use the coroutine code in user-emulation mode? Looking at the meson.build files, we only add the coroutine_*.c to util_ss if 'have_block', and we set have_block = have_system or have_tools. I think (but have not checked) that that means we will build and link the object file into the user-mode binaries if you happen to build them in the same run as system-mode binaries, but won't build them in if you built the user-mode binaries as a separate build. Which is odd and probably worth fixing, but does mean we know that we aren't actually using coroutines in user-mode. (Also user-mode really means Linux or BSD and I think both of those have working ucontext.) thanks -- PMM
Re: Thread safety of coroutine-sigaltstack
On 22.01.21 11:14, Peter Maydell wrote: On Fri, 22 Jan 2021 at 08:50, Max Reitz wrote: On 20.01.21 18:25, Laszlo Ersek wrote: [...] A simple grep for SIGUSR2 seems to indicate that SIGUSR2 is not used by system emulation for anything else, in practice. Is it possible to dedicate SIGUSR2 explicitly to coroutine-sigaltstack, and set up the action beforehand, from some init function that executes on a "central" thread, before qemu_coroutine_new() is ever called? I wrote a patch to that effect, but just before sending I wondered whether SIGUSR2 cannot be registered by the “guest” in user-mode emulation, and whether that would then break coroutines from there on. (I have no experience dealing with user-mode emulation, but it does look like the guest can just register handlers for any signal but SIGSEGV and SIGBUS.) Yes, SIGUSR2 is for the guest in user-emulation mode. OTOH do we even use the coroutine code in user-emulation mode? Looking at the meson.build files, we only add the coroutine_*.c to util_ss if 'have_block', and we set have_block = have_system or have_tools. I think (but have not checked) that that means we will build and link the object file into the user-mode binaries if you happen to build them in the same run as system-mode binaries, but won't build them in if you built the user-mode binaries as a separate build. Which is odd and probably worth fixing, but does mean we know that we aren't actually using coroutines in user-mode. (Also user-mode really means Linux or BSD and I think both of those have working ucontext.) OK, great. Thanks for looking that up. Max
Re: [PATCHv8 3/3] arm-virt: add secure pl061 for reset/power down
On Fri, Jan 22, 2021 at 10:09:35AM +, Peter Maydell wrote:
> On Fri, 22 Jan 2021 at 08:29, Andrew Jones wrote:
> >
> > On Wed, Jan 20, 2021 at 12:27:48PM +0300, Maxim Uvarov wrote:
> > > Add secure pl061 for reset/power down machine from
> > > the secure world (Arm Trusted Firmware). Connect it
> > > with gpio-pwr driver.
> > >
> > > Signed-off-by: Maxim Uvarov
> > > ---
> > > hw/arm/Kconfig| 1 +
> > > hw/arm/virt.c | 47 +++
> > > include/hw/arm/virt.h | 2 ++
> > > 3 files changed, 50 insertions(+)
> > >
> > > diff --git a/hw/arm/Kconfig b/hw/arm/Kconfig
> > > index 0a242e4c5d..13cc42dcc8 100644
> > > --- a/hw/arm/Kconfig
> > > +++ b/hw/arm/Kconfig
> > > @@ -17,6 +17,7 @@ config ARM_VIRT
> > > select PL011 # UART
> > > select PL031 # RTC
> > > select PL061 # GPIO
> > > +select GPIO_PWR
> > > select PLATFORM_BUS
> > > select SMBIOS
> > > select VIRTIO_MMIO
> > > diff --git a/hw/arm/virt.c b/hw/arm/virt.c
> > > index c427ce5f81..060a5f492e 100644
> > > --- a/hw/arm/virt.c
> > > +++ b/hw/arm/virt.c
> > > @@ -153,6 +153,7 @@ static const MemMapEntry base_memmap[] = {
> > > [VIRT_ACPI_GED] = { 0x0908, ACPI_GED_EVT_SEL_LEN },
> > > [VIRT_NVDIMM_ACPI] ={ 0x0909, NVDIMM_ACPI_IO_LEN},
> > > [VIRT_PVTIME] = { 0x090a, 0x0001 },
> > > +[VIRT_SECURE_GPIO] ={ 0x090b, 0x1000 },
> > > [VIRT_MMIO] = { 0x0a00, 0x0200 },
> > > /* ...repeating for a total of NUM_VIRTIO_TRANSPORTS, each of that
> > > size */
> > > [VIRT_PLATFORM_BUS] = { 0x0c00, 0x0200 },
> > > @@ -841,6 +842,43 @@ static void create_gpio_keys(const VirtMachineState
> > > *vms,
> > > "gpios", phandle, 3, 0);
> > > }
> > >
> > > +#define SECURE_GPIO_POWEROFF 0
> > > +#define SECURE_GPIO_REBOOT 1
> > > +
> > > +static void create_gpio_pwr(const VirtMachineState *vms,
> >
> > This function is specific to the secure view. I think it should have
> > "secure" in its name.
> >
> > > +DeviceState *pl061_dev,
> > > +uint32_t phandle)
> > > +{
> > > +DeviceState *gpio_pwr_dev;
> > > +
> > > +/* gpio-pwr */
> > > +gpio_pwr_dev = sysbus_create_simple("gpio-pwr", -1, NULL);
> >
> > Should this device be in secure memory?
>
> It's not in any memory at all -- -1 as the address argument
> to sysbus_create_simple() means "no MMIO regions to map". The
> only way it's connected to the rest of the system is via the
> secure-only PL061, so the NS world can't get at it.
>
> (sysbus_create_simple("device", -1, NULL) is equivalent to:
> dev = qdev_new("device");
> sysbus_realize_and_unref(SYSBUS_DEVICE(dev), &error_fatal);
> )
>
Thanks, I should have looked more closely at that.
With the function name change to include "secure".
Reviewed-by: Andrew Jones
Re: [PATCH] gitlab-ci.yml: Use the whole tree as artifacts to speed up the CI
On Fri, Jan 22, 2021 at 11:07:22AM +0100, Thomas Huth wrote:
> Currently, our check-system-* jobs are recompiling the whole sources
> again. This happens due to the fact that the jobs are checking out
> the whole source tree and required submodules again, and only try
> to use the "build" directory with the binaries and object files
> as an artifact from the previous stage - which simply does not work
> anymore (with the current version of meson). Due to some changed
> time stamps, meson is always trying to rebuild the whole tree.
This used to work in the past didn't it ? Did something change in
meson to break this, or have we just not noticed before.
> So instead of trying to marry a freshly checked out source tree
> with the pre-built binaries in these jobs, let's simply pass the
> whole source including the submodules and the build tree as artifact
> to the test jobs. That way timestamps get preserved and there is
> no rebuild of the sources anymore. This saves ca. 15 - 20 minutes
> of precious CI cycles in each run.
I'm a little worried we might end up hitting the artifact size
limit which is supposedly 1GB on gitlab.com. Im guessing this
must be measuring the compressed size though, as a src checkout
with build dir and .git dir is already way over 1GB.
>
> Signed-off-by: Thomas Huth
> ---
> This is how a job looked like before my patch, running for 42 minutes:
> https://gitlab.com/huth/qemu/-/jobs/978432757
>
> And this is how it looks like afterwards - it just took 18 minutes:
> https://gitlab.com/huth/qemu/-/jobs/979500316
>
> .gitlab-ci.d/containers.yml | 1 +
> .gitlab-ci.yml | 40 +
> 2 files changed, 28 insertions(+), 13 deletions(-)
>
> diff --git a/.gitlab-ci.yml b/.gitlab-ci.yml
> index e2f9c99e27..d55280661f 100644
> --- a/.gitlab-ci.yml
> +++ b/.gitlab-ci.yml
> @@ -39,7 +39,6 @@ include:
>image: $CI_REGISTRY_IMAGE/qemu/$IMAGE:latest
>script:
> - cd build
> -- find . -type f -exec touch {} +
> - make $MAKE_CHECK_ARGS
>
> .acceptance_template: &acceptance_definition
> @@ -83,8 +82,7 @@ build-system-alpine:
>artifacts:
> expire_in: 2 days
> paths:
> - - .git-submodule-status
> - - build
> + - "*"
>
> check-system-alpine:
><<: *native_test_job_definition
> @@ -92,6 +90,7 @@ check-system-alpine:
> - job: build-system-alpine
>artifacts: true
>variables:
> +GIT_CHECKOUT: "false"
> IMAGE: alpine
> MAKE_CHECK_ARGS: check
>
> @@ -101,6 +100,7 @@ acceptance-system-alpine:
> - job: build-system-alpine
>artifacts: true
>variables:
> +GIT_CHECKOUT: "false"
> IMAGE: alpine
> MAKE_CHECK_ARGS: check-acceptance
><<: *acceptance_definition
> @@ -116,7 +116,7 @@ build-system-ubuntu:
>artifacts:
> expire_in: 2 days
> paths:
> - - build
> + - "*"
>
> check-system-ubuntu:
><<: *native_test_job_definition
> @@ -124,6 +124,7 @@ check-system-ubuntu:
> - job: build-system-ubuntu
>artifacts: true
>variables:
> +GIT_CHECKOUT: "false"
> IMAGE: ubuntu2004
> MAKE_CHECK_ARGS: check
>
> @@ -133,6 +134,7 @@ acceptance-system-ubuntu:
> - job: build-system-ubuntu
>artifacts: true
>variables:
> +GIT_CHECKOUT: "false"
> IMAGE: ubuntu2004
> MAKE_CHECK_ARGS: check-acceptance
><<: *acceptance_definition
> @@ -148,7 +150,7 @@ build-system-debian:
>artifacts:
> expire_in: 2 days
> paths:
> - - build
> + - "*"
>
> check-system-debian:
><<: *native_test_job_definition
> @@ -156,6 +158,7 @@ check-system-debian:
> - job: build-system-debian
>artifacts: true
>variables:
> +GIT_CHECKOUT: "false"
> IMAGE: debian-amd64
> MAKE_CHECK_ARGS: check
>
> @@ -170,7 +173,7 @@ build-tools-and-docs-debian:
>artifacts:
> expire_in: 2 days
> paths:
> - - build
> + - "*"
>
> acceptance-system-debian:
><<: *native_test_job_definition
> @@ -178,6 +181,7 @@ acceptance-system-debian:
> - job: build-system-debian
>artifacts: true
>variables:
> +GIT_CHECKOUT: "false"
> IMAGE: debian-amd64
> MAKE_CHECK_ARGS: check-acceptance
><<: *acceptance_definition
> @@ -194,7 +198,7 @@ build-system-fedora:
>artifacts:
> expire_in: 2 days
> paths:
> - - build
> + - "*"
>
> check-system-fedora:
><<: *native_test_job_definition
> @@ -202,6 +206,7 @@ check-system-fedora:
> - job: build-system-fedora
>artifacts: true
>variables:
> +GIT_CHECKOUT: "false"
> IMAGE: fedora
> MAKE_CHECK_ARGS: check
>
> @@ -211,6 +216,7 @@ acceptance-system-fedora:
> - job: build-system-fedora
>artifacts: true
>variables:
> +GIT_CHECKOUT: "false"
> IMAGE: fedora
> MAKE_CHECK_ARGS: check-acceptance
><<: *acceptance_definition
> @@ -226,7 +232,7 @@ build-system-centos:
>artifacts:
>
[PATCH] coroutine-sigaltstack: Keep SIGUSR2 handler up
Modifying signal handlers is a process-global operation. When two
threads run coroutine-sigaltstack's qemu_coroutine_new() concurrently,
they may interfere with each other: One of them may revert the SIGUSR2
handler back to the default between the other thread setting up
coroutine_trampoline() as the handler and raising SIGUSR2. That SIGUSR2
will then lead to the process exiting.
Outside of coroutine-sigaltstack, qemu does not use SIGUSR2. We can
thus keep the signal handler installed all the time.
CoroutineThreadState.tr_handler tells coroutine_trampoline() whether its
stack is set up so a new coroutine is to be launched (i.e., it should
invoke sigsetjmp()), or not (i.e., the signal came from an external
source and we should just perform the default action, which is to exit
the process).
Note that in user-mode emulation, the guest can register signal handlers
for any signal but SIGSEGV and SIGBUS, so if it registers a SIGUSR2
handler, sigaltstack coroutines will break from then on. However, we do
not use coroutines for user-mode emulation, so that is fine.
Suggested-by: Laszlo Ersek
Signed-off-by: Max Reitz
---
util/coroutine-sigaltstack.c | 56 +++-
1 file changed, 29 insertions(+), 27 deletions(-)
diff --git a/util/coroutine-sigaltstack.c b/util/coroutine-sigaltstack.c
index aade82afb8..2d32afc322 100644
--- a/util/coroutine-sigaltstack.c
+++ b/util/coroutine-sigaltstack.c
@@ -59,6 +59,8 @@ typedef struct {
static pthread_key_t thread_state_key;
+static void coroutine_trampoline(int signal);
+
static CoroutineThreadState *coroutine_get_thread_state(void)
{
CoroutineThreadState *s = pthread_getspecific(thread_state_key);
@@ -80,6 +82,7 @@ static void qemu_coroutine_thread_cleanup(void *opaque)
static void __attribute__((constructor)) coroutine_init(void)
{
+struct sigaction sa;
int ret;
ret = pthread_key_create(&thread_state_key, qemu_coroutine_thread_cleanup);
@@ -87,6 +90,20 @@ static void __attribute__((constructor)) coroutine_init(void)
fprintf(stderr, "unable to create leader key: %s\n", strerror(errno));
abort();
}
+
+/*
+ * Establish the SIGUSR2 signal handler. This is a process-wide
+ * operation, and so will apply to all threads from here on.
+ */
+sa = (struct sigaction) {
+.sa_handler = coroutine_trampoline,
+.sa_flags = SA_ONSTACK,
+};
+
+if (sigaction(SIGUSR2, &sa, NULL) != 0) {
+perror("Unable to install SIGUSR2 handler");
+abort();
+}
}
/* "boot" function
@@ -121,7 +138,17 @@ static void coroutine_trampoline(int signal)
/* Get the thread specific information */
coTS = coroutine_get_thread_state();
self = coTS->tr_handler;
+
+if (!self) {
+/*
+ * This SIGUSR2 came from an external source, not from
+ * qemu_coroutine_new(), so perform the default action.
+ */
+exit(0);
+}
+
coTS->tr_called = 1;
+coTS->tr_handler = NULL;
co = &self->base;
/*
@@ -150,12 +177,9 @@ Coroutine *qemu_coroutine_new(void)
{
CoroutineSigAltStack *co;
CoroutineThreadState *coTS;
-struct sigaction sa;
-struct sigaction osa;
stack_t ss;
stack_t oss;
sigset_t sigs;
-sigset_t osigs;
sigjmp_buf old_env;
/* The way to manipulate stack is with the sigaltstack function. We
@@ -172,24 +196,6 @@ Coroutine *qemu_coroutine_new(void)
co->stack = qemu_alloc_stack(&co->stack_size);
co->base.entry_arg = &old_env; /* stash away our jmp_buf */
-coTS = coroutine_get_thread_state();
-coTS->tr_handler = co;
-
-/*
- * Preserve the SIGUSR2 signal state, block SIGUSR2,
- * and establish our signal handler. The signal will
- * later transfer control onto the signal stack.
- */
-sigemptyset(&sigs);
-sigaddset(&sigs, SIGUSR2);
-pthread_sigmask(SIG_BLOCK, &sigs, &osigs);
-sa.sa_handler = coroutine_trampoline;
-sigfillset(&sa.sa_mask);
-sa.sa_flags = SA_ONSTACK;
-if (sigaction(SIGUSR2, &sa, &osa) != 0) {
-abort();
-}
-
/*
* Set the new stack.
*/
@@ -207,6 +213,8 @@ Coroutine *qemu_coroutine_new(void)
* signal can be delivered the first time sigsuspend() is
* called.
*/
+coTS = coroutine_get_thread_state();
+coTS->tr_handler = co;
coTS->tr_called = 0;
pthread_kill(pthread_self(), SIGUSR2);
sigfillset(&sigs);
@@ -230,12 +238,6 @@ Coroutine *qemu_coroutine_new(void)
sigaltstack(&oss, NULL);
}
-/*
- * Restore the old SIGUSR2 signal handler and mask
- */
-sigaction(SIGUSR2, &osa, NULL);
-pthread_sigmask(SIG_SETMASK, &osigs, NULL);
-
/*
* Now enter the trampoline again, but this time not as a signal
* handler. Instead we jump into it directly. The functionally
--
2.29.2
Re: [PATCH] gitlab-ci.yml: Use the whole tree as artifacts to speed up the CI
On Fri, Jan 22, 2021 at 10:18:33AM +, Daniel P. Berrangé wrote: > On Fri, Jan 22, 2021 at 11:07:22AM +0100, Thomas Huth wrote: > > Currently, our check-system-* jobs are recompiling the whole sources > > again. This happens due to the fact that the jobs are checking out > > the whole source tree and required submodules again, and only try > > to use the "build" directory with the binaries and object files > > as an artifact from the previous stage - which simply does not work > > anymore (with the current version of meson). Due to some changed > > time stamps, meson is always trying to rebuild the whole tree. > > This used to work in the past didn't it ? Did something change in > meson to break this, or have we just not noticed before. For to ask, could we address it by using 'meson test --no-rebuild' perhaps ? Regards, Daniel -- |: https://berrange.com -o-https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o-https://fstop138.berrange.com :| |: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|
Re: [PATCH] linux-user/mmap: Avoid asserts for out of range mremap calls
On 1/22/21 10:37 AM, Richard Purdie wrote:
> On Fri, 2021-01-08 at 17:42 +, Richard Purdie wrote:
>> If mremap() is called without the MREMAP_MAYMOVE flag with a start address
>> just before the end of memory (reserved_va) where new_size would exceed
>> it (and GUEST_ADDR_MAX), the assert(end - 1 <= GUEST_ADDR_MAX) in
>> page_set_flags() would trigger.
>>
>> Add an extra guard to the guest_range_valid() checks to prevent this and
>> avoid asserting binaries when reserved_va is set.
>>
>> This meant a bug I was seeing locally now gives the same behaviour
>> regardless of whether reserved_va is set or not.
>>
>> Signed-off-by: Richard Purdie >
>> Index: qemu-5.2.0/linux-user/mmap.c
>> ===
>> --- qemu-5.2.0.orig/linux-user/mmap.c
>> +++ qemu-5.2.0/linux-user/mmap.c
>> @@ -727,7 +727,9 @@ abi_long target_mremap(abi_ulong old_add
>>
>>
>> if (!guest_range_valid(old_addr, old_size) ||
>> ((flags & MREMAP_FIXED) &&
>> - !guest_range_valid(new_addr, new_size))) {
>> + !guest_range_valid(new_addr, new_size)) ||
>> +((flags & MREMAP_MAYMOVE) == 0 &&
>> + !guest_range_valid(old_addr, new_size))) {
>> errno = ENOMEM;
>> return -1;
>> }
>>
>>
>
> Any comments on this? I believe its a valid issue that needs fixing and
> multiple distros appear to be carrying fixes in this area related to
> this.
You forgot to Cc the maintainer who likely missed it due to the huge
traffic:
$ ./scripts/get_maintainer.pl -f linux-user/mmap.c
Laurent Vivier (maintainer:Linux user)
Now Cc'ed.
Regards,
Phil.
Re: [RFC PATCH] linux-user/mmap: Return EFAULT for invalid addresses
Cc'ing maintainer
On 1/22/21 10:37 AM, Richard Purdie wrote:
> On Fri, 2021-01-08 at 17:46 +, Richard Purdie wrote:
>> When using qemu-i386 to run gobject introspection parts of a webkitgtk
>> build using musl as libc on a 64 bit host, it sits in an infinite loop
>> of mremap calls of ever decreasing/increasing addresses.
>>
>> I suspect something in the musl memory allocation code loops indefinitely
>> if it only sees ENOMEM and only exits when it hits EFAULT.
>>
>> According to the docs, trying to mremap outside the address space
>> can/should return EFAULT and changing this allows the build to succeed.
>>
>> There was previous discussion of this as it used to work before qemu 2.11
>> and we've carried hacks to work around it since, this appears to be a
>> better fix of the real issue?
>>
>> Signed-off-by: Richard Purdie >
>> Index: qemu-5.2.0/linux-user/mmap.c
>> ===
>> --- qemu-5.2.0.orig/linux-user/mmap.c
>> +++ qemu-5.2.0/linux-user/mmap.c
>> @@ -727,7 +727,7 @@ abi_long target_mremap(abi_ulong old_add
>> !guest_range_valid(new_addr, new_size)) ||
>> ((flags & MREMAP_MAYMOVE) == 0 &&
>> !guest_range_valid(old_addr, new_size))) {
>> -errno = ENOMEM;
>> +errno = EFAULT;
>> return -1;
>> }
>
> Any comments on this? I believe its a valid issue that needs fixing and
> multiple distros appear to be carrying fixes in this area related to
> this.
>
> Cheers,
>
> Richard
>
>
Re: [PATCH] gitlab-ci.yml: Use the whole tree as artifacts to speed up the CI
On 1/22/21 11:18 AM, Daniel P. Berrangé wrote: > On Fri, Jan 22, 2021 at 11:07:22AM +0100, Thomas Huth wrote: >> Currently, our check-system-* jobs are recompiling the whole sources >> again. This happens due to the fact that the jobs are checking out >> the whole source tree and required submodules again, and only try >> to use the "build" directory with the binaries and object files >> as an artifact from the previous stage - which simply does not work >> anymore (with the current version of meson). Due to some changed >> time stamps, meson is always trying to rebuild the whole tree. > > This used to work in the past didn't it ? Did something change in > meson to break this, or have we just not noticed before. Likely https://github.com/mesonbuild/meson/pull/7900/ Kludge: https://lists.gnu.org/archive/html/qemu-devel/2021-01/msg05491.html
Re: [PATCH] util/log: flush TB cache when log level changes
Hi Pavel, On 1/22/21 11:03 AM, Pavel Dovgalyuk wrote: > Sometimes we need to collect the translation logs starting > from some point of the execution. Some TB listings may > be missed in this case, when blocks were translated before. > This patch clears TB cache to allow re-translation of such > code blocks. > > Signed-off-by: Pavel Dovgalyuk > --- > accel/tcg/translate-all.c |8 > include/sysemu/tcg.h |1 + > stubs/meson.build |1 + > stubs/tcg.c | 12 > util/log.c|3 +++ > 5 files changed, 25 insertions(+) > create mode 100644 stubs/tcg.c ... > /* > * Formerly ifdef DEBUG_TB_CHECK. These debug functions are user-mode-only, > * so in order to prevent bit rot we compile them unconditionally in > user-mode, > diff --git a/include/sysemu/tcg.h b/include/sysemu/tcg.h > index 00349fb18a..7415f11022 100644 > --- a/include/sysemu/tcg.h > +++ b/include/sysemu/tcg.h > @@ -9,6 +9,7 @@ > #define SYSEMU_TCG_H > > void tcg_exec_init(unsigned long tb_size, int splitwx); > +void tb_flush_all(void); Why not declare in "exec/exec-all.h"?
Re: [PULL 0/9] s390x updates
On Thu, 21 Jan 2021 at 12:16, Cornelia Huck wrote: > > The following changes since commit 954b83f13236d21b4116b93a726ea36b5dc2d303: > > Merge remote-tracking branch > 'remotes/huth-gitlab/tags/pull-request-2021-01-20' into staging (2021-01-20 > 17:44:31 +) > > are available in the Git repository at: > > https://gitlab.com/cohuck/qemu.git tags/s390x-20210121 > > for you to fetch changes up to e6a80232f4087e8c7ec253f573319f69165b859d: > > s390x: Use strpadcpy for copying vm name (2021-01-21 11:19:45 +0100) > > > s390x updates: > - headers update to Linux 5.11-rc2 > - fix tcg emulation for some instructions that are generated by > clang Linux kernel builds > - vfio-ccw: wire up the device unplug notification mechanism > - fix a gcc 11 warning > Applied, thanks. Please update the changelog at https://wiki.qemu.org/ChangeLog/6.0 for any user-visible changes. -- PMM
[PATCH v2] hw/core/resettable: make in-reset state false during exit phase call
Move the reset count decrement from "just after" to "just before"
calling the exit phase handler. The goal is to make
resettable_is_in_reset() returning false during the handler execution.
This simplifies reset handling in resettable devices.
Typically, a function that updates the device state will just need
to read the current reset state and not anymore treat the "in
a reset-exit transition" special case.
As a side note, this patch also fixes the fact that the reset count was
not decremented in case of recursive reset.
Signed-off-by: Damien Hedde
Buglink: https://bugs.launchpad.net/qemu/+bug/1905297
Reported-by: Michael Peter
--
Hi,
Following our discussion:
https://lists.nongnu.org/archive/html/qemu-devel/2021-01/msg01013.html
here's my proposal to fix Michael's bug on a global scope.
I flaged it v2 because I've taken Philippe's remarks there:
https://lists.nongnu.org/archive/html/qemu-devel/2020-12/msg00635.html
I've also changed the patch title, I think it is better this way.
Thanks,
Damien
Cc: f4...@amsat.org
Cc: peter.mayd...@linaro.org
Cc: alist...@alistair23.me
Cc: edgar.igles...@gmail.com
---
docs/devel/reset.rst | 6 +++---
hw/core/resettable.c | 3 +--
2 files changed, 4 insertions(+), 5 deletions(-)
diff --git a/docs/devel/reset.rst b/docs/devel/reset.rst
index abea1102dc..021a7277a2 100644
--- a/docs/devel/reset.rst
+++ b/docs/devel/reset.rst
@@ -210,9 +210,9 @@ Polling the reset state
Resettable interface provides the ``resettable_is_in_reset()`` function.
This function returns true if the object parameter is currently under reset.
-An object is under reset from the beginning of the *init* phase to the end of
-the *exit* phase. During all three phases, the function will return that the
-object is in reset.
+An object is under reset from the beginning of the *init* phase to the *exit*
+phase. During *init* and *hold* phase only, the function will return that the
+object is in reset. The state is changed just before calling the *exit* method.
This function may be used if the object behavior has to be adapted
while in reset state. For example if a device has an irq input,
diff --git a/hw/core/resettable.c b/hw/core/resettable.c
index 96a99ce39e..c3df75c6ba 100644
--- a/hw/core/resettable.c
+++ b/hw/core/resettable.c
@@ -201,12 +201,11 @@ static void resettable_phase_exit(Object *obj, void
*opaque, ResetType type)
resettable_child_foreach(rc, obj, resettable_phase_exit, NULL, type);
assert(s->count > 0);
-if (s->count == 1) {
+if (--s->count == 0) {
trace_resettable_phase_exit_exec(obj, obj_typename, !!rc->phases.exit);
if (rc->phases.exit && !resettable_get_tr_func(rc, obj)) {
rc->phases.exit(obj);
}
-s->count = 0;
}
s->exit_phase_in_progress = false;
trace_resettable_phase_exit_end(obj, obj_typename, s->count);
--
2.29.2
Re: [PATCH] replay: fix replay of the interrupts
On 1/19/21 1:39 PM, Pavel Dovgalyuk wrote:
> Sometimes interrupt event comes at the same time with
> the virtual timers. In this case replay tries to proceed
> the timers, because deadline for them is zero.
> This patch allows processing interrupts and exceptions
> by entering the vCPU execution loop, when deadline is zero,
> but checkpoint associated with virtual timers is not ready
> to be replayed.
>
> Signed-off-by: Pavel Dovgalyuk
> ---
> accel/tcg/tcg-cpus-icount.c |8 +++-
> 1 file changed, 7 insertions(+), 1 deletion(-)
>
> diff --git a/accel/tcg/tcg-cpus-icount.c b/accel/tcg/tcg-cpus-icount.c
> index 9f45432275..a6d2bb8a88 100644
> --- a/accel/tcg/tcg-cpus-icount.c
> +++ b/accel/tcg/tcg-cpus-icount.c
> @@ -81,7 +81,13 @@ void icount_handle_deadline(void)
> int64_t deadline = qemu_clock_deadline_ns_all(QEMU_CLOCK_VIRTUAL,
>QEMU_TIMER_ATTR_ALL);
>
> -if (deadline == 0) {
> +/*
> + * Instructions, interrupts, and exceptions are processed in cpu-exec.
> + * Don't interrupt cpu thread, when these events are waiting
> + * (i.e., there is no checkpoint)
> + */
> +if (deadline == 0
> +&& (replay_mode == REPLAY_MODE_RECORD || replay_has_checkpoint())) {
LGTM, but Cc'ing Peter/Alex just in case :)
> icount_notify_aio_contexts();
> }
> }
>
>
Re: [PATCH] util/log: flush TB cache when log level changes
On 22.01.2021 13:32, Philippe Mathieu-Daudé wrote: Hi Pavel, On 1/22/21 11:03 AM, Pavel Dovgalyuk wrote: Sometimes we need to collect the translation logs starting from some point of the execution. Some TB listings may be missed in this case, when blocks were translated before. This patch clears TB cache to allow re-translation of such code blocks. Signed-off-by: Pavel Dovgalyuk --- accel/tcg/translate-all.c |8 include/sysemu/tcg.h |1 + stubs/meson.build |1 + stubs/tcg.c | 12 util/log.c|3 +++ 5 files changed, 25 insertions(+) create mode 100644 stubs/tcg.c ... /* * Formerly ifdef DEBUG_TB_CHECK. These debug functions are user-mode-only, * so in order to prevent bit rot we compile them unconditionally in user-mode, diff --git a/include/sysemu/tcg.h b/include/sysemu/tcg.h index 00349fb18a..7415f11022 100644 --- a/include/sysemu/tcg.h +++ b/include/sysemu/tcg.h @@ -9,6 +9,7 @@ #define SYSEMU_TCG_H void tcg_exec_init(unsigned long tb_size, int splitwx); +void tb_flush_all(void); Why not declare in "exec/exec-all.h"? It includes cpu.h, which is not available for all tools, that use logs. Pavel Dovgalyuk
Re: [PATCH v2 2/8] nbd: allow reconnect on open, with corresponding new options
21.01.2021 04:44, Eric Blake wrote:
On 11/30/20 7:40 AM, Vladimir Sementsov-Ogievskiy wrote:
Note: currently, using new option with long timeout in qmp command
blockdev-add is not good idea, as qmp interface is blocking, so,
don't add it now, let's add it later after
"monitor: Optionally run handlers in coroutines" series merged.
If I'm not mistaken, that landed as of eb94b81a94. Is it just the
commit message that needs an update, or does this patch need a respin?
Oh yes, you are right. I think the most reasonable thing is to keep this patch
in separate (for simple backporting to downstream without Kevin's series), and
add qmp support for the feature as additional new patch. Will do it on respin.
Signed-off-by: Vladimir Sementsov-Ogievskiy
---
block/nbd.c | 115 +---
1 file changed, 92 insertions(+), 23 deletions(-)
@@ -474,6 +484,11 @@ nbd_co_establish_connection(BlockDriverState *bs, Error
**errp)
s->wait_connect = true;
qemu_coroutine_yield();
+if (!s->connect_thread) {
+error_setg(errp, "Connection attempt cancelled by other operation");
+return NULL;
+}
Does this need to use atomics for proper access to s->connect_thread
across threads? Or are all the operations done by other coroutines but
within the same thread, so we are safe?
s->connect_thread is not accessed from connect_thread_func, so in this way we
are safe. And variables shared between connect_thread_func and other driver code
are protected by mutex.
What about accessing nbd bds from different threads.. In my observation, all the
code is written in assumption that everything inside block-driver may be called
from different coroutines but from one thread.. And we have a lot of s->*
variables that are not atomic and not protected by mutexes, and all this works
somehow:)
I remember Paolo answered me somewhere in mailing list, that actually,
everything in block drivers and block/io must be thread-safe.. But I don't see
this thread-safety in current code, so don't introduce it for new variables.
@@ -624,10 +645,15 @@ static coroutine_fn void
nbd_reconnect_attempt(BDRVNBDState *s)
bdrv_inc_in_flight(s->bs);
out:
-s->connect_status = ret;
-error_free(s->connect_err);
-s->connect_err = NULL;
-error_propagate(&s->connect_err, local_err);
+if (s->connect_status == -ETIMEDOUT) {
+/* Don't rewrite timeout error by following cancel-provoked error */
Maybe:
/* Don't propagate a timeout error caused by a job cancellation. */
No, we want to keep ETIMEOUT
+static void open_timer_cb(void *opaque)
+{
+BDRVNBDState *s = opaque;
+
+if (!s->connect_status) {
+/* First attempt was not finished. We should set an error */
+s->connect_status = -ETIMEDOUT;
+error_setg(&s->connect_err, "First connection attempt is cancelled by "
+ "timeout");
+}
+
+nbd_teardown_connection_async(s->bs);
+open_timer_del(s);
+}
+
+static void open_timer_init(BDRVNBDState *s, uint64_t expire_time_ns)
+{
+assert(!s->open_timer && s->state == NBD_CLIENT_OPENING);
+s->open_timer = aio_timer_new(bdrv_get_aio_context(s->bs),
+ QEMU_CLOCK_REALTIME,
+ SCALE_NS,
+ open_timer_cb, s);
+timer_mod(s->open_timer, expire_time_ns);
+}
+
@@ -2180,6 +2235,14 @@ static QemuOptsList nbd_runtime_opts = {
"future requests before a successful reconnect will "
"immediately fail. Default 0",
},
+{
+.name = "open-timeout",
+.type = QEMU_OPT_NUMBER,
+.help = "In seconds. If zero, nbd driver tries to establish "
+"connection only once, on fail open fails. If non-zero, "
If zero, the nbd driver tries the connection only once, and fails to
open if the connection fails.
+"nbd driver may do several attempts until success or "
+"@open-timeout seconds passed. Default 0",
If non-zero, the nbd driver will repeat connection attempts until
successful or until @open-timeout seconds have elapsed.
+},
Where is the QMP counterpart for setting this option?
Absent (as described in commit msg). Will do in a separate patch.
{ /* end of list */ }
},
};
@@ -2235,6 +2298,7 @@ static int nbd_process_options(BlockDriverState *bs,
QDict *options,
}
s->reconnect_delay = qemu_opt_get_number(opts, "reconnect-delay", 0);
+s->open_timeout = qemu_opt_get_number(opts, "open-timeout", 0);
ret = 0;
@@ -2268,6 +2332,11 @@ static int nbd_open(BlockDriverState *bs, QDict *options, int flags,
bdrv_inc_in_flight(bs);
aio_co_schedule(bdrv_get_aio_context(bs), s->connection_co);
+if (s->open_timeout) {
+open_timer_init(s, qemu_clock_get_ns(QEMU_CLO
[PATCH v2 0/4] meson: Try to clarify TCG / TCI options for new users
Since v2: - Included Thomas suggestions Some new users get confused between 'TCG' and 'TCI' and enable TCI when TCG is better for they needs. Try to clarify it is better to not use TCI when native backend is available. Note, before Meson, warnings were summarized at the end of ./configure. Now they are displayed earlier, and likely missed IMHO. No clue how to improve that :/ Based-on: <20210121095616.1471869-1-phi...@redhat.com> Philippe Mathieu-Daudé (4): meson: Explicit TCG backend used meson: Warn when TCI is selected but TCG backend is available configure: Improve TCI feature description configure: Reword --enable-tcg-interpreter as --disable-native-tcg configure | 5 +++-- meson.build | 11 +-- 2 files changed, 12 insertions(+), 4 deletions(-) -- 2.26.2
[PATCH v2 1/4] meson: Explicit TCG backend used
Reviewed-by: Thomas Huth
Reviewed-by: Stefan Weil
Signed-off-by: Philippe Mathieu-Daudé
---
meson.build | 8 ++--
1 file changed, 6 insertions(+), 2 deletions(-)
diff --git a/meson.build b/meson.build
index 8535a83fb70..d5b76150e49 100644
--- a/meson.build
+++ b/meson.build
@@ -229,7 +229,7 @@
if not get_option('tcg').disabled()
if cpu not in supported_cpus
if 'CONFIG_TCG_INTERPRETER' in config_host
- warning('Unsupported CPU @0@, will use TCG with TCI
(experimental)'.format(cpu))
+ warning('Unsupported CPU @0@, will use TCG with TCI (experimental and
slow)'.format(cpu))
else
error('Unsupported CPU @0@, try --enable-tcg-interpreter'.format(cpu))
endif
@@ -2419,8 +2419,12 @@
endif
summary_info += {'TCG support': config_all.has_key('CONFIG_TCG')}
if config_all.has_key('CONFIG_TCG')
+ if config_host.has_key('CONFIG_TCG_INTERPRETER')
+summary_info += {'TCG backend': 'TCG interpreter (experimental)'}
+ else
+summary_info += {'TCG backend': 'native (@0@)'.format(cpu)}
+ endif
summary_info += {'TCG debug enabled':
config_host.has_key('CONFIG_DEBUG_TCG')}
- summary_info += {'TCG interpreter':
config_host.has_key('CONFIG_TCG_INTERPRETER')}
endif
summary_info += {'target list': ' '.join(target_dirs)}
if have_system
--
2.26.2
[PATCH v2 3/4] configure: Improve TCI feature description
Users might want to enable all features, without realizing some features have negative effect. Mention the TCI feature is slow and experimental, hoping it will be selected knowingly. Suggested-by: Thomas Huth Signed-off-by: Philippe Mathieu-Daudé --- configure | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/configure b/configure index 9f016b06b54..71bdc523aa0 100755 --- a/configure +++ b/configure @@ -1753,7 +1753,7 @@ Advanced options (experts only): --with-trace-file=NAME Full PATH,NAME of file to store traces Default:trace- --disable-slirp disable SLIRP userspace network connectivity - --enable-tcg-interpreter enable TCG with bytecode interpreter (TCI) + --enable-tcg-interpreter enable TCG with bytecode interpreter (experimental and slow) --enable-malloc-trim enable libc malloc_trim() for memory optimization --oss-libpath to OSS library --cpu=CPUBuild for host CPU [$cpu] -- 2.26.2
[PATCH v2 2/4] meson: Warn when TCI is selected but TCG backend is available
Some new users get confused with 'TCG' and 'TCI', and enable TCI
support expecting to enable TCG.
Emit a warning when native TCG backend is available on the
host architecture, mentioning this is a suboptimal configuration.
Reviewed-by: Stefan Weil
Reviewed-by: Thomas Huth
Signed-off-by: Philippe Mathieu-Daudé
---
meson.build | 3 +++
1 file changed, 3 insertions(+)
diff --git a/meson.build b/meson.build
index d5b76150e49..d3df5fa3516 100644
--- a/meson.build
+++ b/meson.build
@@ -234,6 +234,9 @@
error('Unsupported CPU @0@, try --enable-tcg-interpreter'.format(cpu))
endif
endif
+ if 'CONFIG_TCG_INTERPRETER' in config_host and cpu in supported_cpus
+warning('Experimental TCI requested while native TCG is available on @0@,
suboptimal performance expected'.format(cpu))
+ endif
accelerators += 'CONFIG_TCG'
config_host += { 'CONFIG_TCG': 'y' }
endif
--
2.26.2
[RFC PATCH v2 4/4] configure: Reword --enable-tcg-interpreter as --disable-native-tcg
Users might want to enable all features, without realizing some features have negative effect. Rename '--enable-tcg-interpreter' as '--disable-native-tcg' to avoid user selecting this feature without understanding it. '--enable-tcg-interpreter' is kept in for backward compability with scripts. Suggested-by: Thomas Huth Signed-off-by: Philippe Mathieu-Daudé --- RFC so it can be discarded from the series configure | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/configure b/configure index 71bdc523aa0..5e56fa76499 100755 --- a/configure +++ b/configure @@ -1121,7 +1121,8 @@ for opt do ;; --disable-tcg-interpreter) tcg_interpreter="no" ;; - --enable-tcg-interpreter) tcg_interpreter="yes" + --enable-tcg-interpreter) # backward compatibility + --disable-native-tcg) tcg_interpreter="yes" ;; --disable-cap-ng) cap_ng="disabled" ;; @@ -1753,7 +1754,7 @@ Advanced options (experts only): --with-trace-file=NAME Full PATH,NAME of file to store traces Default:trace- --disable-slirp disable SLIRP userspace network connectivity - --enable-tcg-interpreter enable TCG with bytecode interpreter (experimental and slow) + --disable-native-tcg enable TCG with bytecode interpreter (experimental and slow) --enable-malloc-trim enable libc malloc_trim() for memory optimization --oss-libpath to OSS library --cpu=CPUBuild for host CPU [$cpu] -- 2.26.2
Re: [PATCH v2 06/36] block: BdrvChildClass: add .get_parent_aio_context handler
19.01.2021 19:38, Kevin Wolf wrote: Am 18.01.2021 um 18:36 hat Vladimir Sementsov-Ogievskiy geschrieben: 18.01.2021 18:13, Kevin Wolf wrote: Am 27.11.2020 um 15:44 hat Vladimir Sementsov-Ogievskiy geschrieben: Add new handler to get aio context and implement it in all child classes. Add corresponding public interface to be used soon. Signed-off-by: Vladimir Sementsov-Ogievskiy Hm, are you going to introduce cases where parent and child context don't match, or why is this a useful function? Even if so, I feel it shouldn't be any of the child's business what AioContext the parent uses. Well, maybe the rest of the series will answer this. It's for the following patch, to not pass parent (as opaque) with it's class, and with its ctx in separate. Just to simplify the interface of the function, we are going to work with a lot. In a way, the result is nicer because we already assume that ctx is the parent context when we move things to different AioContexts. So it's more consistent to just directly take it from the parent. At the same time, it also complicates things a bit because now we need a defined state in the middle of an operation that adds, removes or replaces a child. I guess I still to make more progress in the review of this series until I see how you're using the interface. Hm. I'll take this opportunity to say, that the terminology that calls graph edge "BdrvChild" always leads to the mess between parents and children.. "child_class" is a class of parent.. list of parents is list of children... It all would be a lot simpler to understand if BdrvChild was BdrvEdge or something like this. Yeah, that would probably be clearer now that we actually use it to work with both ends of the edge. And BdrvNode instead of BlockDriverState, I guess. Do you think, we can just rename them? Or it would be too painful for developers, who get used to current names? I can make patches -- Best regards, Vladimir
Re: [PATCH v2 06/36] block: BdrvChildClass: add .get_parent_aio_context handler
Am 22.01.2021 um 12:04 hat Vladimir Sementsov-Ogievskiy geschrieben: > 19.01.2021 19:38, Kevin Wolf wrote: > > Am 18.01.2021 um 18:36 hat Vladimir Sementsov-Ogievskiy geschrieben: > > > 18.01.2021 18:13, Kevin Wolf wrote: > > > > Am 27.11.2020 um 15:44 hat Vladimir Sementsov-Ogievskiy geschrieben: > > > > > Add new handler to get aio context and implement it in all child > > > > > classes. Add corresponding public interface to be used soon. > > > > > > > > > > Signed-off-by: Vladimir Sementsov-Ogievskiy > > > > > > > > Hm, are you going to introduce cases where parent and child context > > > > don't match, or why is this a useful function? > > > > > > > > Even if so, I feel it shouldn't be any of the child's business what > > > > AioContext the parent uses. > > > > > > > > Well, maybe the rest of the series will answer this. > > > > > > It's for the following patch, to not pass parent (as opaque) with it's > > > class, and with its ctx in separate. Just to simplify the interface of > > > the function, we are going to work with a lot. > > > > In a way, the result is nicer because we already assume that ctx is the > > parent context when we move things to different AioContexts. So it's > > more consistent to just directly take it from the parent. > > > > At the same time, it also complicates things a bit because now we need a > > defined state in the middle of an operation that adds, removes or > > replaces a child. > > > > I guess I still to make more progress in the review of this series until > > I see how you're using the interface. > > > > > Hm. I'll take this opportunity to say, that the terminology that calls > > > graph edge "BdrvChild" always leads to the mess between parents and > > > children.. "child_class" is a class of parent.. list of parents is > > > list of children... It all would be a lot simpler to understand if > > > BdrvChild was BdrvEdge or something like this. > > > > Yeah, that would probably be clearer now that we actually use it to > > work with both ends of the edge. And BdrvNode instead of > > BlockDriverState, I guess. > > Do you think, we can just rename them? Or it would be too painful for > developers, > who get used to current names? I can make patches I think getting used to new names wouldn't be too bad. I would be more afraid of the merge conflicts. Not sure, but it might in the category that we would do it differently if we were starting over, but maybe not worth changing now. Kevin
Re: [PATCH v2 06/36] block: BdrvChildClass: add .get_parent_aio_context handler
22.01.2021 14:18, Kevin Wolf wrote: Am 22.01.2021 um 12:04 hat Vladimir Sementsov-Ogievskiy geschrieben: 19.01.2021 19:38, Kevin Wolf wrote: Am 18.01.2021 um 18:36 hat Vladimir Sementsov-Ogievskiy geschrieben: 18.01.2021 18:13, Kevin Wolf wrote: Am 27.11.2020 um 15:44 hat Vladimir Sementsov-Ogievskiy geschrieben: Add new handler to get aio context and implement it in all child classes. Add corresponding public interface to be used soon. Signed-off-by: Vladimir Sementsov-Ogievskiy Hm, are you going to introduce cases where parent and child context don't match, or why is this a useful function? Even if so, I feel it shouldn't be any of the child's business what AioContext the parent uses. Well, maybe the rest of the series will answer this. It's for the following patch, to not pass parent (as opaque) with it's class, and with its ctx in separate. Just to simplify the interface of the function, we are going to work with a lot. In a way, the result is nicer because we already assume that ctx is the parent context when we move things to different AioContexts. So it's more consistent to just directly take it from the parent. At the same time, it also complicates things a bit because now we need a defined state in the middle of an operation that adds, removes or replaces a child. I guess I still to make more progress in the review of this series until I see how you're using the interface. Hm. I'll take this opportunity to say, that the terminology that calls graph edge "BdrvChild" always leads to the mess between parents and children.. "child_class" is a class of parent.. list of parents is list of children... It all would be a lot simpler to understand if BdrvChild was BdrvEdge or something like this. Yeah, that would probably be clearer now that we actually use it to work with both ends of the edge. And BdrvNode instead of BlockDriverState, I guess. Do you think, we can just rename them? Or it would be too painful for developers, who get used to current names? I can make patches I think getting used to new names wouldn't be too bad. I would be more afraid of the merge conflicts. Not sure, but it might in the category that we would do it differently if we were starting over, but maybe not worth changing now. Hmm yes, such rename will add a year of uncomfortable patch backporting.. -- Best regards, Vladimir
Re: [Qemu-devel] [PATCH v2 07/11] chardev: Let IOReadHandler use unsigned type
On Fri, 2018-10-12 at 02:22 +0200, Philippe Mathieu-Daudé wrote: > The number of bytes can not be negative nor zero. > > Fixed 2 format string: > - hw/char/spapr_vty.c > - hw/usb/ccid-card-passthru.c > > Suggested-by: Paolo Bonzini > Signed-off-by: Philippe Mathieu-Daudé > Acked-by: Alberto Garcia Sorry to drag up an old patch series. As far as I can see this series was never applied. I suspect a better way of solving the issue may have been found? If so can anyone point me at that change? I ask since CVE-2018-18438 is marked as affecting all qemu versions (https://nvd.nist.gov/vuln/detail/CVE-2018-18438). If it was fixed, the version mask could be updated. If the fix wasn't deemed worthwhile for some reason that is also fine and I can mark this one as such in our system. I'm being told we only need one of the patches in this series which I also don't believe as I suspect we either need the set or none of them! Any info would be most welcome. Cheers, Richard
Re: [PATCH v7 00/11] Rework iotests/check
Am 20.01.2021 um 21:52 hat Eric Blake geschrieben: > On 1/16/21 7:44 AM, Vladimir Sementsov-Ogievskiy wrote: > > Hi all! > > > > These series has 3 goals: > > > > - get rid of group file (to forget about rebase and in-list conflicts) > > - introduce human-readable names for tests > > - rewrite check into python > > > > v7: > > - fix wording and grammar > > - satisfy python linters > > - move argv interfaces all into one in new check script > > - support '-n' == '--dry-run' option > > - update check-block to run check with correct PYTHON > > I'd really like a second reviewer on 7-11, but I'm queueing 1-6 on my > NBD tree now. Oh, you already sent a pull request? Having 6 in without the rest is not a good state. We now have the group info duplicated and one of them is ignored, but will become the meaningful copy later. We need to be careful to not let them diverge now. I hope the rest is fine so we can switch over quickly, otherwise I'd prefer to revert 6 and redo it from the then current state when we merge the whole series. Kevin
Re: [PATCH v2 3/4] configure: Improve TCI feature description
On 22/01/2021 11.58, Philippe Mathieu-Daudé wrote: Users might want to enable all features, without realizing some features have negative effect. Mention the TCI feature is slow and experimental, hoping it will be selected knowingly. Suggested-by: Thomas Huth Signed-off-by: Philippe Mathieu-Daudé --- configure | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/configure b/configure index 9f016b06b54..71bdc523aa0 100755 --- a/configure +++ b/configure @@ -1753,7 +1753,7 @@ Advanced options (experts only): --with-trace-file=NAME Full PATH,NAME of file to store traces Default:trace- --disable-slirp disable SLIRP userspace network connectivity - --enable-tcg-interpreter enable TCG with bytecode interpreter (TCI) + --enable-tcg-interpreter enable TCG with bytecode interpreter (experimental and slow) I'd prefer if we could keep the "TCI" in there ... I remember having grep'ed for "tci" in the help output in the past, so I think it would be good to keep the TLA here. Maybe just put "TCI, slow" in the parantheses and omit "experimental"? Thomas
Re: [RFC PATCH v2 4/4] configure: Reword --enable-tcg-interpreter as --disable-native-tcg
On 22/01/2021 11.58, Philippe Mathieu-Daudé wrote: Users might want to enable all features, without realizing some features have negative effect. Rename '--enable-tcg-interpreter' as '--disable-native-tcg' to avoid user selecting this feature without understanding it. '--enable-tcg-interpreter' is kept in for backward compability with scripts. Suggested-by: Thomas Huth Signed-off-by: Philippe Mathieu-Daudé --- RFC so it can be discarded from the series configure | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/configure b/configure index 71bdc523aa0..5e56fa76499 100755 --- a/configure +++ b/configure @@ -1121,7 +1121,8 @@ for opt do ;; --disable-tcg-interpreter) tcg_interpreter="no" ;; - --enable-tcg-interpreter) tcg_interpreter="yes" + --enable-tcg-interpreter) # backward compatibility + --disable-native-tcg) tcg_interpreter="yes" ;; --disable-cap-ng) cap_ng="disabled" ;; @@ -1753,7 +1754,7 @@ Advanced options (experts only): --with-trace-file=NAME Full PATH,NAME of file to store traces Default:trace- --disable-slirp disable SLIRP userspace network connectivity - --enable-tcg-interpreter enable TCG with bytecode interpreter (experimental and slow) + --disable-native-tcg enable TCG with bytecode interpreter (experimental and slow) The more I think about it, the more I like the idea. Reviewed-by: Thomas Huth
Re: [PATCH v7 00/11] Rework iotests/check
22.01.2021 14:27, Kevin Wolf wrote: Am 20.01.2021 um 21:52 hat Eric Blake geschrieben: On 1/16/21 7:44 AM, Vladimir Sementsov-Ogievskiy wrote: Hi all! These series has 3 goals: - get rid of group file (to forget about rebase and in-list conflicts) - introduce human-readable names for tests - rewrite check into python v7: - fix wording and grammar - satisfy python linters - move argv interfaces all into one in new check script - support '-n' == '--dry-run' option - update check-block to run check with correct PYTHON I'd really like a second reviewer on 7-11, but I'm queueing 1-6 on my NBD tree now. Oh, you already sent a pull request? Having 6 in without the rest is not a good state. We now have the group info duplicated and one of them is ignored, but will become the meaningful copy later. We need to be careful to not let them diverge now. I hope the rest is fine so we can switch over quickly, otherwise I'd prefer to revert 6 and redo it from the then current state when we merge the whole series. I can make a new version now with tiny fixes suggested by Eric if it is convenient. (and keep --color suggestion for a separate follow-up) -- Best regards, Vladimir
Re: [PATCH v3 12/21] linux-user/aarch64: Implement PR_TAGGED_ADDR_ENABLE
On Fri, 15 Jan 2021 at 22:47, Richard Henderson
wrote:
>
> This is the prctl bit that controls whether syscalls accept tagged
> addresses. See Documentation/arm64/tagged-address-abi.rst in the
> linux kernel.
>
> Signed-off-by: Richard Henderson
> ---
> linux-user/aarch64/target_syscall.h | 4
> target/arm/cpu-param.h | 3 +++
> target/arm/cpu.h| 23 +++
> linux-user/syscall.c| 25 +
> target/arm/cpu.c| 3 +++
> 5 files changed, 58 insertions(+)
>
> diff --git a/linux-user/aarch64/target_syscall.h
> b/linux-user/aarch64/target_syscall.h
> index 3194e6b009..820601dfcc 100644
> --- a/linux-user/aarch64/target_syscall.h
> +++ b/linux-user/aarch64/target_syscall.h
> @@ -30,4 +30,8 @@ struct target_pt_regs {
> # define TARGET_PR_PAC_APDBKEY (1 << 3)
> # define TARGET_PR_PAC_APGAKEY (1 << 4)
>
> +#define TARGET_PR_SET_TAGGED_ADDR_CTRL 55
> +#define TARGET_PR_GET_TAGGED_ADDR_CTRL 56
> +# define TARGET_PR_TAGGED_ADDR_ENABLE (1UL << 0)
Stray extra space.
Otherwise
Reviewed-by: Peter Maydell
thanks
-- PMM
Re: [PATCH] util/log: flush TB cache when log level changes
Pavel Dovgalyuk writes:
> Sometimes we need to collect the translation logs starting
> from some point of the execution. Some TB listings may
> be missed in this case, when blocks were translated before.
> This patch clears TB cache to allow re-translation of such
> code blocks.
>
> Signed-off-by: Pavel Dovgalyuk
> ---
> accel/tcg/translate-all.c |8
> include/sysemu/tcg.h |1 +
> stubs/meson.build |1 +
> stubs/tcg.c | 12
> util/log.c|3 +++
> 5 files changed, 25 insertions(+)
> create mode 100644 stubs/tcg.c
>
> diff --git a/accel/tcg/translate-all.c b/accel/tcg/translate-all.c
> index e9de6ff9dd..3acb227c57 100644
> --- a/accel/tcg/translate-all.c
> +++ b/accel/tcg/translate-all.c
> @@ -1461,6 +1461,14 @@ void tb_flush(CPUState *cpu)
> }
> }
>
> +void tb_flush_all(void)
> +{
> +CPUState *cpu;
> +CPU_FOREACH(cpu) {
> +tb_flush(cpu);
> +}
> +}
> +
This isn't needed - tb_flush flushes all translations although it does
need to be executed in a CPU context to do so.
> /*
> * Formerly ifdef DEBUG_TB_CHECK. These debug functions are user-mode-only,
> * so in order to prevent bit rot we compile them unconditionally in
> user-mode,
> diff --git a/include/sysemu/tcg.h b/include/sysemu/tcg.h
> index 00349fb18a..7415f11022 100644
> --- a/include/sysemu/tcg.h
> +++ b/include/sysemu/tcg.h
> @@ -9,6 +9,7 @@
> #define SYSEMU_TCG_H
>
> void tcg_exec_init(unsigned long tb_size, int splitwx);
> +void tb_flush_all(void);
>
> #ifdef CONFIG_TCG
> extern bool tcg_allowed;
> diff --git a/stubs/meson.build b/stubs/meson.build
> index 80b1d81a31..95e70f8542 100644
> --- a/stubs/meson.build
> +++ b/stubs/meson.build
> @@ -38,6 +38,7 @@ stub_ss.add(files('set-fd-handler.c'))
> stub_ss.add(files('sysbus.c'))
> stub_ss.add(files('target-get-monitor-def.c'))
> stub_ss.add(files('target-monitor-defs.c'))
> +stub_ss.add(files('tcg.c'))
> stub_ss.add(files('tpm.c'))
> stub_ss.add(files('trace-control.c'))
> stub_ss.add(files('uuid.c'))
> diff --git a/stubs/tcg.c b/stubs/tcg.c
> new file mode 100644
> index 00..775a748c77
> --- /dev/null
> +++ b/stubs/tcg.c
> @@ -0,0 +1,12 @@
> +/*
> + * TCG stubs
> + *
> + * This work is licensed under the terms of the GNU GPL, version 2 or later.
> + * See the COPYING file in the top-level directory.
> + */
> +
> +#include "sysemu/tcg.h"
> +
> +void tb_flush_all(void)
> +{
> +}
> diff --git a/util/log.c b/util/log.c
> index 2ee1500bee..2ff342a91b 100644
> --- a/util/log.c
> +++ b/util/log.c
> @@ -26,6 +26,7 @@
> #include "trace/control.h"
> #include "qemu/thread.h"
> #include "qemu/lockable.h"
> +#include "sysemu/tcg.h"
>
> static char *logfilename;
> static QemuMutex qemu_logfile_mutex;
> @@ -84,6 +85,8 @@ void qemu_set_log(int log_flags)
> #ifdef CONFIG_TRACE_LOG
> qemu_loglevel |= LOG_TRACE;
> #endif
> +tb_flush_all();
> +
I would call tb_flush(current_cpu) or first_cpu here. But two things:
- I'm not sure you have a CPU at all times qemu_set_log is called
- It seems overly aggressive to throw away all translations every time
the log level is changed. I would define a mask in log.h and have
something like:
if (log_flags & LOG_TRANSLATION) {
tb_flush();
}
> /*
> * In all cases we only log if qemu_loglevel is set.
> * Also:
--
Alex Bennée
Re: [PATCH v7 07/11] iotests: add findtests.py
Am 16.01.2021 um 14:44 hat Vladimir Sementsov-Ogievskiy geschrieben:
> Add python script with new logic of searching for tests:
>
> Current ./check behavior:
> - tests are named [0-9][0-9][0-9]
> - tests must be registered in group file (even if test doesn't belong
>to any group, like 142)
>
> Behavior of findtests.py:
> - group file is dropped
> - tests are all files in tests/ subdirectory (except for .out files),
>so it's not needed more to "register the test", just create it with
>appropriate name in tests/ subdirectory. Old names like
>[0-9][0-9][0-9] (in root iotests directory) are supported too, but
>not recommended for new tests
> - groups are parsed from '# group: ' line inside test files
> - optional file group.local may be used to define some additional
>groups for downstreams
> - 'disabled' group is used to temporary disable tests. So instead of
>commenting tests in old 'group' file you now can add them to
>disabled group with help of 'group.local' file
> - selecting test ranges like 5-15 are not supported more
>(to support restarting failed ./check command from the middle of the
> process, new argument is added: --start-from)
>
> Benefits:
> - no rebase conflicts in group file on patch porting from branch to
>branch
> - no conflicts in upstream, when different series want to occupy same
>test number
> - meaningful names for test files
>For example, with digital number, when some person wants to add some
>test about block-stream, he most probably will just create a new
>test. But if there would be test-block-stream test already, he will
>at first look at it and may be just add a test-case into it.
>And anyway meaningful names are better.
>
> This commit don't update check behavior (which will be done in further
> commit), still, the documentation changed like new behavior is already
> here. Let's live with this small inconsistency for the following few
> commits, until final change.
>
> The file findtests.py is self-executable and may be used for debugging
> purposes.
As Eric mentioned, this isn't accurate any more.
You mentioned using it as a way to debug things. I assume this is now
covered by the dry run option?
> Signed-off-by: Vladimir Sementsov-Ogievskiy
> ---
> docs/devel/testing.rst | 50 +-
> tests/qemu-iotests/findtests.py | 159
> 2 files changed, 208 insertions(+), 1 deletion(-)
> create mode 100644 tests/qemu-iotests/findtests.py
> +def add_group_file(self, fname: str) -> None:
> +with open(fname) as f:
> +for line in f:
> +line = line.strip()
> +
> +if (not line) or line[0] == '#':
> +continue
> +
> +words = line.split()
> +test_file = self.parse_test_name(words[0])
> +groups = words[1:]
The previous version still had this:
+if test_file not in self.all_tests:
+print(f'Warning: {fname}: "{test_file}" test is not found.'
+ ' Skip.')
+continue
Why did you remove it? I found this useful when I had a wrong test name
in my group.local. Now it's silently ignored.
> +for g in groups:
> +self.groups[g].add(test_file)
Kevin
Re: [PATCH v3 13/21] linux-user/aarch64: Implement PR_MTE_TCF and PR_MTE_TAG
On Fri, 15 Jan 2021 at 22:47, Richard Henderson
wrote:
>
> These prctl fields are required for the function of MTE.
>
> Signed-off-by: Richard Henderson
> ---
> linux-user/aarch64/target_syscall.h | 9 ++
> linux-user/syscall.c| 44 +
> 2 files changed, 53 insertions(+)
>
> diff --git a/linux-user/aarch64/target_syscall.h
> b/linux-user/aarch64/target_syscall.h
> index 820601dfcc..76f6c3391d 100644
> --- a/linux-user/aarch64/target_syscall.h
> +++ b/linux-user/aarch64/target_syscall.h
> @@ -33,5 +33,14 @@ struct target_pt_regs {
> #define TARGET_PR_SET_TAGGED_ADDR_CTRL 55
> #define TARGET_PR_GET_TAGGED_ADDR_CTRL 56
> # define TARGET_PR_TAGGED_ADDR_ENABLE (1UL << 0)
> +/* MTE tag check fault modes */
> +# define TARGET_PR_MTE_TCF_SHIFT 1
> +# define TARGET_PR_MTE_TCF_NONE(0UL << TARGET_PR_MTE_TCF_SHIFT)
> +# define TARGET_PR_MTE_TCF_SYNC(1UL << TARGET_PR_MTE_TCF_SHIFT)
> +# define TARGET_PR_MTE_TCF_ASYNC (2UL << TARGET_PR_MTE_TCF_SHIFT)
> +# define TARGET_PR_MTE_TCF_MASK(3UL << TARGET_PR_MTE_TCF_SHIFT)
> +/* MTE tag inclusion mask */
> +# define TARGET_PR_MTE_TAG_SHIFT 3
> +# define TARGET_PR_MTE_TAG_MASK(0xUL << TARGET_PR_MTE_TAG_SHIFT)
Reviewed-by: Peter Maydell
thanks
-- PMM
Re: [PATCH v7 07/11] iotests: add findtests.py
Am 16.01.2021 um 14:44 hat Vladimir Sementsov-Ogievskiy geschrieben: > +if 'disabled' not in groups and 'disabled' not in exclude_groups: > +# Don't want to modify function argument, so create new list. > +exclude_groups = exclude_groups + ['disabled'] Oops, forgot the other comment I wanted to make: This would only have been needed if you had turned exclude_groups into a Sequence. Now that it's still a list, copying the list isn't strictly necessary. Kevin
Re: [PATCH v3 12/21] linux-user/aarch64: Implement PR_TAGGED_ADDR_ENABLE
On Fri, 15 Jan 2021 at 22:47, Richard Henderson
wrote:
>
> This is the prctl bit that controls whether syscalls accept tagged
> addresses. See Documentation/arm64/tagged-address-abi.rst in the
> linux kernel.
> +#ifdef TARGET_TAGGED_ADDRESSES
> +/**
> + * cpu_untagged_addr:
> + * @cs: CPU context
> + * @x: tagged address
> + *
> + * Remove any address tag from @x. This is explicitly related to the
> + * linux syscall TIF_TAGGED_ADDR setting, not TBI in general.
> + *
> + * There should be a better place to put this, but we need this in
> + * include/exec/cpu_ldst.h, and not some place linux-user specific.
> + */
> +static inline target_ulong cpu_untagged_addr(CPUState *cs, target_ulong x)
> +{
> +ARMCPU *cpu = ARM_CPU(cs);
> +return x & cpu->env.untagged_addr_mask;
> +}
> +#endif
Forgot to mention: this only does the right thing on addresses
in the lower half of the address space. I guess that's mostly
OK for our purposes? It probably means that if a guest program
deliberately dereferences a bad address in the top half of the
address space we'll report the wrong (ie different to what a real
kernel reports) address value to it in the SEGV signal handler.
The kernel's "untagged_addr()" implementation:
https://elixir.bootlin.com/linux/latest/source/arch/arm64/include/asm/memory.h#L203
slightly confusingly does "untag the addr if it's in the userspace
half, leave the tag bits alone if in the kernel half".
thanks
-- PMM
Re: [Qemu-devel] [PATCH v2 07/11] chardev: Let IOReadHandler use unsigned type
+-- On Fri, 22 Jan 2021, Richard Purdie wrote --+ | If so can anyone point me at that change? | | I ask since CVE-2018-18438 is marked as affecting all qemu versions | (https://nvd.nist.gov/vuln/detail/CVE-2018-18438). | | If it was fixed, the version mask could be updated. If the fix wasn't deemed | worthwhile for some reason that is also fine and I can mark this one as such | in our system. I'm being told we only need one of the patches in this series | which I also don't believe as I suspect we either need the set or none of | them! | | Any info would be most welcome. -> https://lists.gnu.org/archive/html/qemu-devel/2018-10/msg02239.html -> https://lists.gnu.org/archive/html/qemu-devel/2018-10/msg02231.html * Yes, the type change fix had come up during patch reviews above, and this series implemented the change. * Series is required IIUC, didn't realise it's not merged. Thank you. -- Prasad J Pandit / Red Hat Product Security Team 8685 545E B54C 486B C6EB 271E E285 8B5A F050 DE8D
Re: [PATCH v7 07/11] iotests: add findtests.py
22.01.2021 14:48, Kevin Wolf wrote:
Am 16.01.2021 um 14:44 hat Vladimir Sementsov-Ogievskiy geschrieben:
Add python script with new logic of searching for tests:
Current ./check behavior:
- tests are named [0-9][0-9][0-9]
- tests must be registered in group file (even if test doesn't belong
to any group, like 142)
Behavior of findtests.py:
- group file is dropped
- tests are all files in tests/ subdirectory (except for .out files),
so it's not needed more to "register the test", just create it with
appropriate name in tests/ subdirectory. Old names like
[0-9][0-9][0-9] (in root iotests directory) are supported too, but
not recommended for new tests
- groups are parsed from '# group: ' line inside test files
- optional file group.local may be used to define some additional
groups for downstreams
- 'disabled' group is used to temporary disable tests. So instead of
commenting tests in old 'group' file you now can add them to
disabled group with help of 'group.local' file
- selecting test ranges like 5-15 are not supported more
(to support restarting failed ./check command from the middle of the
process, new argument is added: --start-from)
Benefits:
- no rebase conflicts in group file on patch porting from branch to
branch
- no conflicts in upstream, when different series want to occupy same
test number
- meaningful names for test files
For example, with digital number, when some person wants to add some
test about block-stream, he most probably will just create a new
test. But if there would be test-block-stream test already, he will
at first look at it and may be just add a test-case into it.
And anyway meaningful names are better.
This commit don't update check behavior (which will be done in further
commit), still, the documentation changed like new behavior is already
here. Let's live with this small inconsistency for the following few
commits, until final change.
The file findtests.py is self-executable and may be used for debugging
purposes.
As Eric mentioned, this isn't accurate any more.
You mentioned using it as a way to debug things. I assume this is now
covered by the dry run option?
yes
Signed-off-by: Vladimir Sementsov-Ogievskiy
---
docs/devel/testing.rst | 50 +-
tests/qemu-iotests/findtests.py | 159
2 files changed, 208 insertions(+), 1 deletion(-)
create mode 100644 tests/qemu-iotests/findtests.py
+def add_group_file(self, fname: str) -> None:
+with open(fname) as f:
+for line in f:
+line = line.strip()
+
+if (not line) or line[0] == '#':
+continue
+
+words = line.split()
+test_file = self.parse_test_name(words[0])
+groups = words[1:]
The previous version still had this:
+if test_file not in self.all_tests:
+print(f'Warning: {fname}: "{test_file}" test is not found.'
+ ' Skip.')
+continue
Why did you remove it? I found this useful when I had a wrong test name
in my group.local. Now it's silently ignored.
Because now we use parse_test_name which will raise ValueError, so we will not
go to this if anyway.
So, wrong name will not be silently ignored, check will fail, and you'll have
to fix group file.. It is suitable?
+for g in groups:
+self.groups[g].add(test_file)
Kevin
--
Best regards,
Vladimir
Re: [PATCH v7 07/11] iotests: add findtests.py
22.01.2021 14:49, Kevin Wolf wrote: Am 16.01.2021 um 14:44 hat Vladimir Sementsov-Ogievskiy geschrieben: +if 'disabled' not in groups and 'disabled' not in exclude_groups: +# Don't want to modify function argument, so create new list. +exclude_groups = exclude_groups + ['disabled'] Oops, forgot the other comment I wanted to make: This would only have been needed if you had turned exclude_groups into a Sequence. Now that it's still a list, copying the list isn't strictly necessary. I just think that such side effects (changing function arguments when it is not part of function intention) are better to avoid. -- Best regards, Vladimir
Re: [PATCH v3 12/21] linux-user/aarch64: Implement PR_TAGGED_ADDR_ENABLE
On Fri, 22 Jan 2021 at 11:53, Peter Maydell wrote: > The kernel's "untagged_addr()" implementation: > https://elixir.bootlin.com/linux/latest/source/arch/arm64/include/asm/memory.h#L203 > slightly confusingly does "untag the addr if it's in the userspace > half, leave the tag bits alone if in the kernel half". ...and a kernel person has just explained to me the rationale: TBI is always enabled for userspace and never for the kernel, so "always clear tag bits for a userspace address, never clear them for a kernel address" is the right behaviour. I think we should have the same logic. -- PMM
Re: [PATCH v3 17/21] linux-user/aarch64: Signal SEGV_MTESERR for sync tag check fault
On Fri, 15 Jan 2021 at 22:47, Richard Henderson
wrote:
>
> Signed-off-by: Richard Henderson
> ---
> linux-user/aarch64/target_signal.h | 2 ++
> linux-user/aarch64/cpu_loop.c | 3 +++
> 2 files changed, 5 insertions(+)
>
> diff --git a/linux-user/aarch64/target_signal.h
> b/linux-user/aarch64/target_signal.h
> index ddd73169f0..777fb667fe 100644
> --- a/linux-user/aarch64/target_signal.h
> +++ b/linux-user/aarch64/target_signal.h
> @@ -21,5 +21,7 @@ typedef struct target_sigaltstack {
>
> #include "../generic/signal.h"
>
> +#define TARGET_SEGV_MTESERR 9 /* Synchronous ARM MTE exception */
> +
> #define TARGET_ARCH_HAS_SETUP_FRAME
> #endif /* AARCH64_TARGET_SIGNAL_H */
> diff --git a/linux-user/aarch64/cpu_loop.c b/linux-user/aarch64/cpu_loop.c
> index 7811440c68..6867f0db2b 100644
> --- a/linux-user/aarch64/cpu_loop.c
> +++ b/linux-user/aarch64/cpu_loop.c
> @@ -133,6 +133,9 @@ void cpu_loop(CPUARMState *env)
> case 0x0d ... 0x0f: /* Permission fault, level {1-3} */
> info.si_code = TARGET_SEGV_ACCERR;
> break;
> +case 0x11: /* Synchronous Tag Check Fault */
> +info.si_code = TARGET_SEGV_MTESERR;
> +break;
> default:
> g_assert_not_reached();
> }
> --
> 2.25.1
Reviewed-by: Peter Maydell
thanks
-- PMM
[PATCH V5 1/6] hw/block/nvme: introduce nvme-subsys device
To support multi-path in QEMU NVMe device model, We need to have NVMe
subsystem hierarchy to map controllers and namespaces to a NVMe
subsystem.
This patch introduced a simple nvme-subsys device model. The subsystem
will be prepared with subsystem NQN with provided in
nvme-subsys device:
ex) -device nvme-subsys,id=subsys0: nqn.2019-08.org.qemu:subsys0
Signed-off-by: Minwoo Im
---
hw/block/meson.build | 2 +-
hw/block/nvme-subsys.c | 60 ++
hw/block/nvme-subsys.h | 25 ++
hw/block/nvme.c| 3 +++
4 files changed, 89 insertions(+), 1 deletion(-)
create mode 100644 hw/block/nvme-subsys.c
create mode 100644 hw/block/nvme-subsys.h
diff --git a/hw/block/meson.build b/hw/block/meson.build
index 602ca6c8541d..83ea2d37978d 100644
--- a/hw/block/meson.build
+++ b/hw/block/meson.build
@@ -13,7 +13,7 @@ softmmu_ss.add(when: 'CONFIG_SSI_M25P80', if_true:
files('m25p80.c'))
softmmu_ss.add(when: 'CONFIG_SWIM', if_true: files('swim.c'))
softmmu_ss.add(when: 'CONFIG_XEN', if_true: files('xen-block.c'))
softmmu_ss.add(when: 'CONFIG_SH4', if_true: files('tc58128.c'))
-softmmu_ss.add(when: 'CONFIG_NVME_PCI', if_true: files('nvme.c', 'nvme-ns.c'))
+softmmu_ss.add(when: 'CONFIG_NVME_PCI', if_true: files('nvme.c', 'nvme-ns.c',
'nvme-subsys.c'))
specific_ss.add(when: 'CONFIG_VIRTIO_BLK', if_true: files('virtio-blk.c'))
specific_ss.add(when: 'CONFIG_VHOST_USER_BLK', if_true:
files('vhost-user-blk.c'))
diff --git a/hw/block/nvme-subsys.c b/hw/block/nvme-subsys.c
new file mode 100644
index ..aa82911b951c
--- /dev/null
+++ b/hw/block/nvme-subsys.c
@@ -0,0 +1,60 @@
+/*
+ * QEMU NVM Express Subsystem: nvme-subsys
+ *
+ * Copyright (c) 2021 Minwoo Im
+ *
+ * This code is licensed under the GNU GPL v2. Refer COPYING.
+ */
+
+#include "qemu/units.h"
+#include "qemu/osdep.h"
+#include "qemu/uuid.h"
+#include "qemu/iov.h"
+#include "qemu/cutils.h"
+#include "qapi/error.h"
+#include "hw/qdev-properties.h"
+#include "hw/qdev-core.h"
+#include "hw/block/block.h"
+#include "block/aio.h"
+#include "block/accounting.h"
+#include "sysemu/sysemu.h"
+#include "hw/pci/pci.h"
+#include "nvme.h"
+#include "nvme-subsys.h"
+
+static void nvme_subsys_setup(NvmeSubsystem *subsys)
+{
+snprintf((char *)subsys->subnqn, sizeof(subsys->subnqn),
+ "nqn.2019-08.org.qemu:%s", subsys->parent_obj.id);
+}
+
+static void nvme_subsys_realize(DeviceState *dev, Error **errp)
+{
+NvmeSubsystem *subsys = NVME_SUBSYS(dev);
+
+nvme_subsys_setup(subsys);
+}
+
+static void nvme_subsys_class_init(ObjectClass *oc, void *data)
+{
+DeviceClass *dc = DEVICE_CLASS(oc);
+
+set_bit(DEVICE_CATEGORY_STORAGE, dc->categories);
+
+dc->realize = nvme_subsys_realize;
+dc->desc = "Virtual NVMe subsystem";
+}
+
+static const TypeInfo nvme_subsys_info = {
+.name = TYPE_NVME_SUBSYS,
+.parent = TYPE_DEVICE,
+.class_init = nvme_subsys_class_init,
+.instance_size = sizeof(NvmeSubsystem),
+};
+
+static void nvme_subsys_register_types(void)
+{
+type_register_static(&nvme_subsys_info);
+}
+
+type_init(nvme_subsys_register_types)
diff --git a/hw/block/nvme-subsys.h b/hw/block/nvme-subsys.h
new file mode 100644
index ..40f06a4c7db0
--- /dev/null
+++ b/hw/block/nvme-subsys.h
@@ -0,0 +1,25 @@
+/*
+ * QEMU NVM Express Subsystem: nvme-subsys
+ *
+ * Copyright (c) 2021 Minwoo Im
+ *
+ * This code is licensed under the GNU GPL v2. Refer COPYING.
+ */
+
+#ifndef NVME_SUBSYS_H
+#define NVME_SUBSYS_H
+
+#define TYPE_NVME_SUBSYS "nvme-subsys"
+#define NVME_SUBSYS(obj) \
+OBJECT_CHECK(NvmeSubsystem, (obj), TYPE_NVME_SUBSYS)
+
+#define NVME_SUBSYS_MAX_CTRLS 32
+
+typedef struct NvmeCtrl NvmeCtrl;
+typedef struct NvmeNamespace NvmeNamespace;
+typedef struct NvmeSubsystem {
+DeviceState parent_obj;
+uint8_t subnqn[256];
+} NvmeSubsystem;
+
+#endif /* NVME_SUBSYS_H */
diff --git a/hw/block/nvme.c b/hw/block/nvme.c
index 21aec90637fa..aabccdf36f4b 100644
--- a/hw/block/nvme.c
+++ b/hw/block/nvme.c
@@ -25,6 +25,7 @@
* mdts=,zoned.append_size_limit= \
* -device nvme-ns,drive=,bus=,nsid=,\
* zoned=
+ * -device nvme-subsys,id=
*
* Note cmb_size_mb denotes size of CMB in MB. CMB is assumed to be at
* offset 0 in BAR2 and supports only WDS, RDS and SQS for now. By default, the
@@ -38,6 +39,8 @@
*
* The PMR will use BAR 4/5 exclusively.
*
+ * To place controller(s) and namespace(s) to a subsystem, then provide
+ * nvme-subsys device as above.
*
* nvme device parameters
* ~~
--
2.17.1
[PATCH V5 0/6] hw/block/nvme: support multi-path for ctrl/ns
Hello,
Here's fifth patch series for the support of NVMe subsystem scheme with
multi-controller and namespace sharing in a subsystem.
This series has applied review comments from the previous series,
mostly from Keith's review. Thanks Keith!
Here's test result with a simple 'nvme list -v' command from this model
with adding a ZNS example with subsys.
-device nvme-subsys,id=subsys0 \
-device nvme,serial=foo,id=nvme0,subsys=subsys0 \
-device nvme,serial=bar,id=nvme1,subsys=subsys0 \
-device nvme,serial=baz,id=nvme2,subsys=subsys0 \
-device nvme-ns,id=ns1,drive=drv10,nsid=1,subsys=subsys0 \
-device nvme-ns,id=ns2,drive=drv11,nsid=2,bus=nvme2 \
\
-device nvme,serial=qux,id=nvme3 \
-device nvme-ns,id=ns3,drive=drv12,nsid=3,bus=nvme3 \
\
-device nvme-subsys,id=subsys1 \
-device nvme,serial=quux,id=nvme4,subsys=subsys1 \
-device nvme-ns,id=ns4,drive=drv13,nsid=1,subsys=subsys1,zoned=true \
root@vm:~/work# nvme list -v
NVM Express Subsystems
SubsystemSubsystem-NQN
Controllers
nvme-subsys1 nqn.2019-08.org.qemu:subsys0
nvme0, nvme1, nvme2
nvme-subsys3 nqn.2019-08.org.qemu:qux
nvme3
nvme-subsys4 nqn.2019-08.org.qemu:subsys1
nvme4
NVM Express Controllers
Device SN MN FR
TxPort AddressSubsystemNamespaces
-- --
nvme0foo QEMU NVMe Ctrl 1.0
pcie :00:06.0 nvme-subsys1 nvme1c0n1
nvme1bar QEMU NVMe Ctrl 1.0
pcie :00:07.0 nvme-subsys1 nvme1c1n1
nvme2baz QEMU NVMe Ctrl 1.0
pcie :00:08.0 nvme-subsys1 nvme1c2n1, nvme1c2n2
nvme3qux QEMU NVMe Ctrl 1.0
pcie :00:09.0 nvme-subsys3 nvme3n1
nvme4quux QEMU NVMe Ctrl 1.0
pcie :00:0a.0 nvme-subsys4 nvme4c4n1
NVM Express Namespaces
Device NSID Usage Format Controllers
--
nvme1n1 1134.22 MB / 134.22 MB512 B + 0 B nvme0,
nvme1, nvme2
nvme1n2 2268.44 MB / 268.44 MB512 B + 0 B nvme2
nvme3n1 3268.44 MB / 268.44 MB512 B + 0 B nvme3
nvme4n1 1268.44 MB / 268.44 MB512 B + 0 B nvme4
Thanks,
Since V4:
- Code clean-up to snprintf rather than duplicating it and copy.
(Keith)
- Documentation for 'subsys' clean-up. (Keith)
- Remove 'cntlid' param from nvme_init_ctrl(). (Keith)
- Put error_propagate() in nvme_realize(). (Keith)
Since RFC V3:
- Exclude 'deatched' scheme from this series. This will be covered in
the next series by covering all the ns-related admin commands
including ZNS and ns-mgmt. (Niklas)
- Rebased on nvme-next.
- Remove RFC tag from this V4.
Since RFC V2:
- Rebased on nvme-next branch with trivial patches from the previous
version(V2) applied. (Klaus)
- Fix enumeration type name convention with NvmeIdNs prefix. (Klaus)
- Put 'cntlid' to NvmeCtrl instance in nvme_init_ctrl() which was
missed in V2.
- Added 'detached' parameter to nvme-ns device to decide whether to
attach or not to controller(s) in the subsystem. (Klaus)
- Implemented Identify Active Namespace ID List aprt from Identify
Allocated Namespace ID List by removing fall-thru statement.
Since RFC V1:
- Updated namespace sharing scheme to be based on nvme-subsys
hierarchy.
Minwoo Im (6):
hw/block/nvme: introduce nvme-subsys device
hw/block/nvme: support to map controller to a subsystem
hw/block/nvme: add CMIC enum value for Identify Controller
hw/block/nvme: support for multi-controller in subsystem
hw/block/nvme: add NMIC enum value for Identify Namespace
hw/block/nvme: support for shared namespace in subsystem
hw/block/meson.build | 2 +-
hw/block/nvme-ns.c | 23 +++--
hw/block/nvme-ns.h | 7 +++
hw/block/nvme-subsys.c | 106 +
hw/block/nvme-subsys.h | 32 +
hw/block/nvme.c| 72 +---
hw/block/nvme.h| 4 ++
include/block/nvme.h | 8
8 files changed, 242 insertions(+), 12 deletions(-)
create mo
[PATCH V5 3/6] hw/block/nvme: add CMIC enum value for Identify Controller
Added Controller Multi-path I/O and Namespace Sharing Capabilities
(CMIC) field to support multi-controller in the following patches.
This field is in Identify Controller data structure in [76].
Signed-off-by: Minwoo Im
---
include/block/nvme.h | 4
1 file changed, 4 insertions(+)
diff --git a/include/block/nvme.h b/include/block/nvme.h
index e4b918064df9..d6415a869c1c 100644
--- a/include/block/nvme.h
+++ b/include/block/nvme.h
@@ -1034,6 +1034,10 @@ enum NvmeIdCtrlLpa {
NVME_LPA_EXTENDED = 1 << 2,
};
+enum NvmeIdCtrlCmic {
+NVME_CMIC_MULTI_CTRL= 1 << 1,
+};
+
#define NVME_CTRL_SQES_MIN(sqes) ((sqes) & 0xf)
#define NVME_CTRL_SQES_MAX(sqes) (((sqes) >> 4) & 0xf)
#define NVME_CTRL_CQES_MIN(cqes) ((cqes) & 0xf)
--
2.17.1
[PATCH V5 4/6] hw/block/nvme: support for multi-controller in subsystem
We have nvme-subsys and nvme devices mapped together. To support
multi-controller scheme to this setup, controller identifier(id) has to
be managed. Earlier, cntlid(controller id) used to be always 0 because
we didn't have any subsystem scheme that controller id matters.
This patch introduced 'cntlid' attribute to the nvme controller
instance(NvmeCtrl) and make it allocated by the nvme-subsys device
mapped to the controller. If nvme-subsys is not given to the
controller, then it will always be 0 as it was.
Added 'ctrls' array in the nvme-subsys instance to manage attached
controllers to the subsystem with a limit(32). This patch didn't take
list for the controllers to make it seamless with nvme-ns device.
Signed-off-by: Minwoo Im
---
hw/block/nvme-subsys.c | 21 +
hw/block/nvme-subsys.h | 4
hw/block/nvme.c| 29 +
hw/block/nvme.h| 1 +
4 files changed, 55 insertions(+)
diff --git a/hw/block/nvme-subsys.c b/hw/block/nvme-subsys.c
index aa82911b951c..e9d61c993c90 100644
--- a/hw/block/nvme-subsys.c
+++ b/hw/block/nvme-subsys.c
@@ -22,6 +22,27 @@
#include "nvme.h"
#include "nvme-subsys.h"
+int nvme_subsys_register_ctrl(NvmeCtrl *n, Error **errp)
+{
+NvmeSubsystem *subsys = n->subsys;
+int cntlid;
+
+for (cntlid = 0; cntlid < ARRAY_SIZE(subsys->ctrls); cntlid++) {
+if (!subsys->ctrls[cntlid]) {
+break;
+}
+}
+
+if (cntlid == ARRAY_SIZE(subsys->ctrls)) {
+error_setg(errp, "no more free controller id");
+return -1;
+}
+
+subsys->ctrls[cntlid] = n;
+
+return cntlid;
+}
+
static void nvme_subsys_setup(NvmeSubsystem *subsys)
{
snprintf((char *)subsys->subnqn, sizeof(subsys->subnqn),
diff --git a/hw/block/nvme-subsys.h b/hw/block/nvme-subsys.h
index 40f06a4c7db0..4eba50d96a1d 100644
--- a/hw/block/nvme-subsys.h
+++ b/hw/block/nvme-subsys.h
@@ -20,6 +20,10 @@ typedef struct NvmeNamespace NvmeNamespace;
typedef struct NvmeSubsystem {
DeviceState parent_obj;
uint8_t subnqn[256];
+
+NvmeCtrl*ctrls[NVME_SUBSYS_MAX_CTRLS];
} NvmeSubsystem;
+int nvme_subsys_register_ctrl(NvmeCtrl *n, Error **errp);
+
#endif /* NVME_SUBSYS_H */
diff --git a/hw/block/nvme.c b/hw/block/nvme.c
index b525fca14103..3dedefb8ebba 100644
--- a/hw/block/nvme.c
+++ b/hw/block/nvme.c
@@ -4435,6 +4435,9 @@ static void nvme_init_ctrl(NvmeCtrl *n, PCIDevice
*pci_dev)
strpadcpy((char *)id->mn, sizeof(id->mn), "QEMU NVMe Ctrl", ' ');
strpadcpy((char *)id->fr, sizeof(id->fr), "1.0", ' ');
strpadcpy((char *)id->sn, sizeof(id->sn), n->params.serial, ' ');
+
+id->cntlid = n->cntlid;
+
id->rab = 6;
id->ieee[0] = 0x00;
id->ieee[1] = 0x02;
@@ -4481,6 +4484,10 @@ static void nvme_init_ctrl(NvmeCtrl *n, PCIDevice
*pci_dev)
id->psd[0].enlat = cpu_to_le32(0x10);
id->psd[0].exlat = cpu_to_le32(0x4);
+if (n->subsys) {
+id->cmic |= NVME_CMIC_MULTI_CTRL;
+}
+
NVME_CAP_SET_MQES(n->bar.cap, 0x7ff);
NVME_CAP_SET_CQR(n->bar.cap, 1);
NVME_CAP_SET_TO(n->bar.cap, 0xf);
@@ -4495,6 +4502,24 @@ static void nvme_init_ctrl(NvmeCtrl *n, PCIDevice
*pci_dev)
n->bar.intmc = n->bar.intms = 0;
}
+static int nvme_init_subsys(NvmeCtrl *n, Error **errp)
+{
+int cntlid;
+
+if (!n->subsys) {
+return 0;
+}
+
+cntlid = nvme_subsys_register_ctrl(n, errp);
+if (cntlid < 0) {
+return -1;
+}
+
+n->cntlid = cntlid;
+
+return 0;
+}
+
static void nvme_realize(PCIDevice *pci_dev, Error **errp)
{
NvmeCtrl *n = NVME(pci_dev);
@@ -4515,6 +4540,10 @@ static void nvme_realize(PCIDevice *pci_dev, Error
**errp)
return;
}
+if (nvme_init_subsys(n, errp)) {
+error_propagate(errp, local_err);
+return;
+}
nvme_init_ctrl(n, pci_dev);
/* setup a namespace if the controller drive property was given */
diff --git a/hw/block/nvme.h b/hw/block/nvme.h
index 04d4684601fd..b8f5f2d6ffb8 100644
--- a/hw/block/nvme.h
+++ b/hw/block/nvme.h
@@ -134,6 +134,7 @@ typedef struct NvmeCtrl {
NvmeBus bus;
BlockConfconf;
+uint16_tcntlid;
boolqs_created;
uint32_tpage_size;
uint16_tpage_bits;
--
2.17.1
[PATCH V5 6/6] hw/block/nvme: support for shared namespace in subsystem
nvme-ns device is registered to a nvme controller device during the
initialization in nvme_register_namespace() in case that 'bus' property
is given which means it's mapped to a single controller.
This patch introduced a new property 'subsys' just like the controller
device instance did to map a namespace to a NVMe subsystem.
If 'subsys' property is given to the nvme-ns device, it will belong to
the specified subsystem and will be attached to all controllers in that
subsystem by enabling shared namespace capability in NMIC(Namespace
Multi-path I/O and Namespace Capabilities) in Identify Namespace.
Usage:
-device nvme-subsys,id=subsys0
-device nvme,serial=foo,id=nvme0,subsys=subsys0
-device nvme,serial=bar,id=nvme1,subsys=subsys0
-device nvme,serial=baz,id=nvme2,subsys=subsys0
-device nvme-ns,id=ns1,drive=,nsid=1,subsys=subsys0 # Shared
-device nvme-ns,id=ns2,drive=,nsid=2,bus=nvme2 # Non-shared
In the above example, 'ns1' will be shared to 'nvme0' and 'nvme1' in
the same subsystem. On the other hand, 'ns2' will be attached to the
'nvme2' only as a private namespace in that subsystem.
All the namespace with 'subsys' parameter will attach all controllers in
the subsystem to the namespace by default.
Signed-off-by: Minwoo Im
---
hw/block/nvme-ns.c | 23 ++-
hw/block/nvme-ns.h | 7 +++
hw/block/nvme-subsys.c | 25 +
hw/block/nvme-subsys.h | 3 +++
hw/block/nvme.c| 10 +-
5 files changed, 62 insertions(+), 6 deletions(-)
diff --git a/hw/block/nvme-ns.c b/hw/block/nvme-ns.c
index 62b25cf69bfa..9b493f2ead03 100644
--- a/hw/block/nvme-ns.c
+++ b/hw/block/nvme-ns.c
@@ -63,6 +63,10 @@ static int nvme_ns_init(NvmeNamespace *ns, Error **errp)
id_ns->npda = id_ns->npdg = npdg - 1;
+if (nvme_ns_shared(ns)) {
+id_ns->nmic |= NVME_NMIC_NS_SHARED;
+}
+
return 0;
}
@@ -365,16 +369,25 @@ static void nvme_ns_realize(DeviceState *dev, Error
**errp)
return;
}
-if (nvme_register_namespace(n, ns, errp)) {
-error_propagate_prepend(errp, local_err,
-"could not register namespace: ");
-return;
+if (ns->subsys) {
+if (nvme_subsys_register_ns(ns, errp)) {
+error_propagate_prepend(errp, local_err,
+"could not setup namespace to subsys: ");
+return;
+}
+} else {
+if (nvme_register_namespace(n, ns, errp)) {
+error_propagate_prepend(errp, local_err,
+"could not register namespace: ");
+return;
+}
}
-
}
static Property nvme_ns_props[] = {
DEFINE_BLOCK_PROPERTIES(NvmeNamespace, blkconf),
+DEFINE_PROP_LINK("subsys", NvmeNamespace, subsys, TYPE_NVME_SUBSYS,
+ NvmeSubsystem *),
DEFINE_PROP_UINT32("nsid", NvmeNamespace, params.nsid, 0),
DEFINE_PROP_UUID("uuid", NvmeNamespace, params.uuid),
DEFINE_PROP_BOOL("zoned", NvmeNamespace, params.zoned, false),
diff --git a/hw/block/nvme-ns.h b/hw/block/nvme-ns.h
index 293ac990e3f6..929e78861903 100644
--- a/hw/block/nvme-ns.h
+++ b/hw/block/nvme-ns.h
@@ -47,6 +47,8 @@ typedef struct NvmeNamespace {
const uint32_t *iocs;
uint8_t csi;
+NvmeSubsystem *subsys;
+
NvmeIdNsZoned *id_ns_zoned;
NvmeZone*zone_array;
QTAILQ_HEAD(, NvmeZone) exp_open_zones;
@@ -77,6 +79,11 @@ static inline uint32_t nvme_nsid(NvmeNamespace *ns)
return -1;
}
+static inline bool nvme_ns_shared(NvmeNamespace *ns)
+{
+return !!ns->subsys;
+}
+
static inline NvmeLBAF *nvme_ns_lbaf(NvmeNamespace *ns)
{
NvmeIdNs *id_ns = &ns->id_ns;
diff --git a/hw/block/nvme-subsys.c b/hw/block/nvme-subsys.c
index e9d61c993c90..641de33e99fc 100644
--- a/hw/block/nvme-subsys.c
+++ b/hw/block/nvme-subsys.c
@@ -43,6 +43,31 @@ int nvme_subsys_register_ctrl(NvmeCtrl *n, Error **errp)
return cntlid;
}
+int nvme_subsys_register_ns(NvmeNamespace *ns, Error **errp)
+{
+NvmeSubsystem *subsys = ns->subsys;
+NvmeCtrl *n;
+int i;
+
+if (subsys->namespaces[nvme_nsid(ns)]) {
+error_setg(errp, "namespace %d already registerd to subsy %s",
+ nvme_nsid(ns), subsys->parent_obj.id);
+return -1;
+}
+
+subsys->namespaces[nvme_nsid(ns)] = ns;
+
+for (i = 0; i < ARRAY_SIZE(subsys->ctrls); i++) {
+n = subsys->ctrls[i];
+
+if (n && nvme_register_namespace(n, ns, errp)) {
+return -1;
+}
+}
+
+return 0;
+}
+
static void nvme_subsys_setup(NvmeSubsystem *subsys)
{
snprintf((char *)subsys->subnqn, sizeof(subsys->subnqn),
diff --git a/hw/block/nvme-subsys.h b/hw/block/nvme-subsys.h
index 4eba50d96a1d..ccf6a71398d3 100644
--- a/hw/block/nvme-subsys.h
+++ b/hw/block/nvme-subsys.h
@@ -14,6 +14,7 @@
OBJECT_CHECK(NvmeSubsystem, (obj), TYPE_NVME_SUBSYS)
#define NVME_SUBSYS_MAX_CTRLS
[PATCH V5 2/6] hw/block/nvme: support to map controller to a subsystem
nvme controller(nvme) can be mapped to a NVMe subsystem(nvme-subsys).
This patch maps a controller to a subsystem by adding a parameter
'subsys' to the nvme device.
To map a controller to a subsystem, we need to put nvme-subsys first and
then maps the subsystem to the controller:
-device nvme-subsys,id=subsys0
-device nvme,serial=foo,id=nvme0,subsys=subsys0
If 'subsys' property is not given to the nvme controller, then subsystem
NQN will be created with serial (e.g., 'foo' in above example),
Otherwise, it will be based on subsys id (e.g., 'subsys0' in above
example).
Signed-off-by: Minwoo Im
---
hw/block/nvme.c | 30 +-
hw/block/nvme.h | 3 +++
2 files changed, 28 insertions(+), 5 deletions(-)
diff --git a/hw/block/nvme.c b/hw/block/nvme.c
index aabccdf36f4b..b525fca14103 100644
--- a/hw/block/nvme.c
+++ b/hw/block/nvme.c
@@ -22,7 +22,8 @@
* [pmrdev=,] \
* max_ioqpairs=, \
* aerl=, aer_max_queued=, \
- * mdts=,zoned.append_size_limit= \
+ * mdts=,zoned.append_size_limit=, \
+ * subsys= \
* -device nvme-ns,drive=,bus=,nsid=,\
* zoned=
* -device nvme-subsys,id=
@@ -44,6 +45,13 @@
*
* nvme device parameters
* ~~
+ * - `subsys`
+ * NVM Subsystem device. If given, a subsystem NQN will be initialized with
+ *given. Otherwise, will be taken for subsystem NQN.
+ * Also, it will enable multi controller capability represented in Identify
+ * Controller data structure in CMIC (Controller Multi-path I/O and Namesapce
+ * Sharing Capabilities), if given.
+ *
* - `aerl`
* The Asynchronous Event Request Limit (AERL). Indicates the maximum number
* of concurrently outstanding Asynchronous Event Request commands support
@@ -4404,11 +4412,23 @@ static int nvme_init_pci(NvmeCtrl *n, PCIDevice
*pci_dev, Error **errp)
return 0;
}
+static void nvme_init_subnqn(NvmeCtrl *n)
+{
+NvmeSubsystem *subsys = n->subsys;
+NvmeIdCtrl *id = &n->id_ctrl;
+
+if (!subsys) {
+snprintf((char *)id->subnqn, sizeof(id->subnqn),
+ "nqn.2019-08.org.qemu:%s", n->params.serial);
+} else {
+pstrcpy((char *)id->subnqn, sizeof(id->subnqn), (char*)subsys->subnqn);
+}
+}
+
static void nvme_init_ctrl(NvmeCtrl *n, PCIDevice *pci_dev)
{
NvmeIdCtrl *id = &n->id_ctrl;
uint8_t *pci_conf = pci_dev->config;
-char *subnqn;
id->vid = cpu_to_le16(pci_get_word(pci_conf + PCI_VENDOR_ID));
id->ssvid = cpu_to_le16(pci_get_word(pci_conf + PCI_SUBSYSTEM_VENDOR_ID));
@@ -4455,9 +4475,7 @@ static void nvme_init_ctrl(NvmeCtrl *n, PCIDevice
*pci_dev)
id->sgls = cpu_to_le32(NVME_CTRL_SGLS_SUPPORT_NO_ALIGN |
NVME_CTRL_SGLS_BITBUCKET);
-subnqn = g_strdup_printf("nqn.2019-08.org.qemu:%s", n->params.serial);
-strpadcpy((char *)id->subnqn, sizeof(id->subnqn), subnqn, '\0');
-g_free(subnqn);
+nvme_init_subnqn(n);
id->psd[0].mp = cpu_to_le16(0x9c4);
id->psd[0].enlat = cpu_to_le32(0x10);
@@ -4545,6 +4563,8 @@ static Property nvme_props[] = {
DEFINE_BLOCK_PROPERTIES(NvmeCtrl, namespace.blkconf),
DEFINE_PROP_LINK("pmrdev", NvmeCtrl, pmr.dev, TYPE_MEMORY_BACKEND,
HostMemoryBackend *),
+DEFINE_PROP_LINK("subsys", NvmeCtrl, subsys, TYPE_NVME_SUBSYS,
+ NvmeSubsystem *),
DEFINE_PROP_STRING("serial", NvmeCtrl, params.serial),
DEFINE_PROP_UINT32("cmb_size_mb", NvmeCtrl, params.cmb_size_mb, 0),
DEFINE_PROP_UINT32("num_queues", NvmeCtrl, params.num_queues, 0),
diff --git a/hw/block/nvme.h b/hw/block/nvme.h
index dee6092bd45f..04d4684601fd 100644
--- a/hw/block/nvme.h
+++ b/hw/block/nvme.h
@@ -2,6 +2,7 @@
#define HW_NVME_H
#include "block/nvme.h"
+#include "nvme-subsys.h"
#include "nvme-ns.h"
#define NVME_MAX_NAMESPACES 256
@@ -170,6 +171,8 @@ typedef struct NvmeCtrl {
uint8_t zasl;
+NvmeSubsystem *subsys;
+
NvmeNamespace namespace;
NvmeNamespace *namespaces[NVME_MAX_NAMESPACES];
NvmeSQueue **sq;
--
2.17.1
[PATCH V5 5/6] hw/block/nvme: add NMIC enum value for Identify Namespace
Added Namespace Multi-path I/O and Namespace Sharing Capabilities (NMIC)
field to support shared namespace from controller(s).
This field is in Identify Namespace data structure in [30].
Signed-off-by: Minwoo Im
---
include/block/nvme.h | 4
1 file changed, 4 insertions(+)
diff --git a/include/block/nvme.h b/include/block/nvme.h
index d6415a869c1c..ad68cdc2b92d 100644
--- a/include/block/nvme.h
+++ b/include/block/nvme.h
@@ -1203,6 +1203,10 @@ enum NvmeNsIdentifierType {
NVME_NIDT_CSI = 0x04,
};
+enum NvmeIdNsNmic {
+NVME_NMIC_NS_SHARED = 1 << 0,
+};
+
enum NvmeCsi {
NVME_CSI_NVM= 0x00,
NVME_CSI_ZONED = 0x02,
--
2.17.1
Re: Thread safety of coroutine-sigaltstack
On 01/22/21 11:14, Peter Maydell wrote: > On Fri, 22 Jan 2021 at 08:50, Max Reitz wrote: >> >> On 20.01.21 18:25, Laszlo Ersek wrote: >> >> [...] >> >>> A simple grep for SIGUSR2 seems to indicate that SIGUSR2 is not used >>> by system emulation for anything else, in practice. Is it possible >>> to dedicate SIGUSR2 explicitly to coroutine-sigaltstack, and set up >>> the action beforehand, from some init function that executes on a >>> "central" thread, before qemu_coroutine_new() is ever called? >> >> I wrote a patch to that effect, but just before sending I wondered >> whether SIGUSR2 cannot be registered by the "guest" in user-mode >> emulation, and whether that would then break coroutines from there >> on. >> >> (I have no experience dealing with user-mode emulation, but it does >> look like the guest can just register handlers for any signal but >> SIGSEGV and SIGBUS.) > > Yes, SIGUSR2 is for the guest in user-emulation mode. Yes, my grep found those occurrences of SIGUSR2 of course. > OTOH do we even use the coroutine code in user-emulation mode? I assumed not. I assumed coroutines were only used by the block system, and user mode emulation is about running userspace Linux programs -- virtual disks are not emulated for those. > Looking at the meson.build files, we only add the coroutine_*.c to > util_ss if 'have_block', and we set have_block = have_system or > have_tools. I think (but have not checked) that that means we will > build and link the object file into the user-mode binaries if you > happen to build them in the same run as system-mode binaries, but > won't build them in if you built the user-mode binaries as a separate > build. Huh. > Which is odd and probably worth fixing, but does mean we > know that we aren't actually using coroutines in user-mode. Thanks for checking! Laszlo > (Also user-mode really means Linux or BSD and I think both of > those have working ucontext.) > > thanks > -- PMM >
[PATCH] hw/mips: loongson3: Drop 'struct MemmapEntry'
From: Bin Meng
There is already a MemMapEntry type defined in hwaddr.h. Let's drop
the loongson3 defined `struct MemmapEntry` and use the existing one.
Signed-off-by: Bin Meng
---
hw/mips/loongson3_bootp.h | 7 +--
hw/mips/loongson3_virt.c | 6 +++---
2 files changed, 4 insertions(+), 9 deletions(-)
diff --git a/hw/mips/loongson3_bootp.h b/hw/mips/loongson3_bootp.h
index 09f8480abf..d525ab745a 100644
--- a/hw/mips/loongson3_bootp.h
+++ b/hw/mips/loongson3_bootp.h
@@ -228,12 +228,7 @@ enum {
LOADER_PARAM,
};
-struct MemmapEntry {
-hwaddr base;
-hwaddr size;
-};
-
-extern const struct MemmapEntry virt_memmap[];
+extern const MemMapEntry virt_memmap[];
void init_loongson_params(struct loongson_params *lp, void *p,
uint64_t cpu_freq, uint64_t ram_size);
void init_reset_system(struct efi_reset_system_t *reset);
diff --git a/hw/mips/loongson3_virt.c b/hw/mips/loongson3_virt.c
index d4a82fa536..b15071defc 100644
--- a/hw/mips/loongson3_virt.c
+++ b/hw/mips/loongson3_virt.c
@@ -72,7 +72,7 @@
#define RTC_IRQ 1
#define PCIE_IRQ_BASE 2
-const struct MemmapEntry virt_memmap[] = {
+const MemMapEntry virt_memmap[] = {
[VIRT_LOWMEM] = { 0x,0x1000 },
[VIRT_PM] = { 0x1008, 0x100 },
[VIRT_FW_CFG] = { 0x10080100, 0x100 },
@@ -86,13 +86,13 @@ const struct MemmapEntry virt_memmap[] = {
[VIRT_HIGHMEM] = { 0x8000, 0x0 }, /* Variable */
};
-static const struct MemmapEntry loader_memmap[] = {
+static const MemMapEntry loader_memmap[] = {
[LOADER_KERNEL] ={ 0x, 0x400 },
[LOADER_INITRD] ={ 0x0400, 0x0 }, /* Variable */
[LOADER_CMDLINE] = { 0x0ff0, 0x10 },
};
-static const struct MemmapEntry loader_rommap[] = {
+static const MemMapEntry loader_rommap[] = {
[LOADER_BOOTROM] = { 0x1fc0,0x1000 },
[LOADER_PARAM] = { 0x1fc01000, 0x1 },
};
--
2.25.1
[PATCH 0/4] hw/riscv: Clean-ups and map high mmio for PCIe of 'virt' machine
From: Bin Meng This series does the following clean-ups: - Drop 'struct MemmapEntry' - virt: Drop the 'link_up' parameter of gpex_pcie_init() It also adds the following small enhancement to 'virt' machine: - Limit RAM size in a 32-bit system - Map high mmio for PCIe Bin Meng (4): hw/riscv: Drop 'struct MemmapEntry' hw/riscv: virt: Drop the 'link_up' parameter of gpex_pcie_init() hw/riscv: virt: Limit RAM size in a 32-bit system hw/riscv: virt: Map high mmio for PCIe hw/riscv/microchip_pfsoc.c | 9 ++--- hw/riscv/opentitan.c | 9 ++--- hw/riscv/sifive_e.c| 9 ++--- hw/riscv/sifive_u.c| 11 +++--- hw/riscv/spike.c | 9 ++--- hw/riscv/virt.c| 72 ++ 6 files changed, 73 insertions(+), 46 deletions(-) -- 2.25.1
[PATCH 1/4] hw/riscv: Drop 'struct MemmapEntry'
From: Bin Meng
There is already a MemMapEntry type defined in hwaddr.h. Let's drop
the RISC-V defined `struct MemmapEntry` and use the existing one.
Signed-off-by: Bin Meng
---
hw/riscv/microchip_pfsoc.c | 9 +++--
hw/riscv/opentitan.c | 9 +++--
hw/riscv/sifive_e.c| 9 +++--
hw/riscv/sifive_u.c| 11 ---
hw/riscv/spike.c | 9 +++--
hw/riscv/virt.c| 9 +++--
6 files changed, 19 insertions(+), 37 deletions(-)
diff --git a/hw/riscv/microchip_pfsoc.c b/hw/riscv/microchip_pfsoc.c
index e952b49e8c..266f1c3342 100644
--- a/hw/riscv/microchip_pfsoc.c
+++ b/hw/riscv/microchip_pfsoc.c
@@ -86,10 +86,7 @@
* - Register Map/PF_SoC_RegMap_V1_1/MPFS250T/mpfs250t_ioscb_memmap_dri.htm
* describes the complete IOSCB modules memory maps
*/
-static const struct MemmapEntry {
-hwaddr base;
-hwaddr size;
-} microchip_pfsoc_memmap[] = {
+static const MemMapEntry microchip_pfsoc_memmap[] = {
[MICROCHIP_PFSOC_RSVD0] = {0x0, 0x100 },
[MICROCHIP_PFSOC_DEBUG] = { 0x100, 0xf00 },
[MICROCHIP_PFSOC_E51_DTIM] ={ 0x100, 0x2000 },
@@ -182,7 +179,7 @@ static void microchip_pfsoc_soc_realize(DeviceState *dev,
Error **errp)
{
MachineState *ms = MACHINE(qdev_get_machine());
MicrochipPFSoCState *s = MICROCHIP_PFSOC(dev);
-const struct MemmapEntry *memmap = microchip_pfsoc_memmap;
+const MemMapEntry *memmap = microchip_pfsoc_memmap;
MemoryRegion *system_memory = get_system_memory();
MemoryRegion *rsvd0_mem = g_new(MemoryRegion, 1);
MemoryRegion *e51_dtim_mem = g_new(MemoryRegion, 1);
@@ -451,7 +448,7 @@ type_init(microchip_pfsoc_soc_register_types)
static void microchip_icicle_kit_machine_init(MachineState *machine)
{
MachineClass *mc = MACHINE_GET_CLASS(machine);
-const struct MemmapEntry *memmap = microchip_pfsoc_memmap;
+const MemMapEntry *memmap = microchip_pfsoc_memmap;
MicrochipIcicleKitState *s = MICROCHIP_ICICLE_KIT_MACHINE(machine);
MemoryRegion *system_memory = get_system_memory();
MemoryRegion *mem_low = g_new(MemoryRegion, 1);
diff --git a/hw/riscv/opentitan.c b/hw/riscv/opentitan.c
index af3456932f..e168bffe69 100644
--- a/hw/riscv/opentitan.c
+++ b/hw/riscv/opentitan.c
@@ -28,10 +28,7 @@
#include "qemu/units.h"
#include "sysemu/sysemu.h"
-static const struct MemmapEntry {
-hwaddr base;
-hwaddr size;
-} ibex_memmap[] = {
+static const MemMapEntry ibex_memmap[] = {
[IBEX_DEV_ROM] ={ 0x8000, 16 * KiB },
[IBEX_DEV_RAM] ={ 0x1000, 0x1 },
[IBEX_DEV_FLASH] = { 0x2000, 0x8 },
@@ -66,7 +63,7 @@ static const struct MemmapEntry {
static void opentitan_board_init(MachineState *machine)
{
-const struct MemmapEntry *memmap = ibex_memmap;
+const MemMapEntry *memmap = ibex_memmap;
OpenTitanState *s = g_new0(OpenTitanState, 1);
MemoryRegion *sys_mem = get_system_memory();
MemoryRegion *main_mem = g_new(MemoryRegion, 1);
@@ -114,7 +111,7 @@ static void lowrisc_ibex_soc_init(Object *obj)
static void lowrisc_ibex_soc_realize(DeviceState *dev_soc, Error **errp)
{
-const struct MemmapEntry *memmap = ibex_memmap;
+const MemMapEntry *memmap = ibex_memmap;
MachineState *ms = MACHINE(qdev_get_machine());
LowRISCIbexSoCState *s = RISCV_IBEX_SOC(dev_soc);
MemoryRegion *sys_mem = get_system_memory();
diff --git a/hw/riscv/sifive_e.c b/hw/riscv/sifive_e.c
index 59bac4cc9a..f939bcf9ea 100644
--- a/hw/riscv/sifive_e.c
+++ b/hw/riscv/sifive_e.c
@@ -50,10 +50,7 @@
#include "sysemu/sysemu.h"
#include "exec/address-spaces.h"
-static const struct MemmapEntry {
-hwaddr base;
-hwaddr size;
-} sifive_e_memmap[] = {
+static MemMapEntry sifive_e_memmap[] = {
[SIFIVE_E_DEV_DEBUG] ={0x0, 0x1000 },
[SIFIVE_E_DEV_MROM] = { 0x1000, 0x2000 },
[SIFIVE_E_DEV_OTP] = {0x2, 0x2000 },
@@ -77,7 +74,7 @@ static const struct MemmapEntry {
static void sifive_e_machine_init(MachineState *machine)
{
-const struct MemmapEntry *memmap = sifive_e_memmap;
+const MemMapEntry *memmap = sifive_e_memmap;
SiFiveEState *s = RISCV_E_MACHINE(machine);
MemoryRegion *sys_mem = get_system_memory();
@@ -187,7 +184,7 @@ static void sifive_e_soc_init(Object *obj)
static void sifive_e_soc_realize(DeviceState *dev, Error **errp)
{
MachineState *ms = MACHINE(qdev_get_machine());
-const struct MemmapEntry *memmap = sifive_e_memmap;
+const MemMapEntry *memmap = sifive_e_memmap;
SiFiveESoCState *s = RISCV_E_SOC(dev);
MemoryRegion *sys_mem = get_system_memory();
diff --git a/hw/riscv/sifive_u.c b/hw/riscv/sifive_u.c
index 59b61cea01..51e4132fc4 100644
--- a/hw/riscv/sifive_u.c
+++ b/hw/riscv/sifive_u.c
@@ -60,10 +60,7 @@
#include
-static const struct MemmapEntry {
-hwaddr base;
-hwaddr size;
-} sifive_u_me
[PATCH 2/4] hw/riscv: virt: Drop the 'link_up' parameter of gpex_pcie_init()
From: Bin Meng
`link_up` is never used in gpex_pcie_init(). Drop it.
Signed-off-by: Bin Meng
---
hw/riscv/virt.c | 14 +++---
1 file changed, 7 insertions(+), 7 deletions(-)
diff --git a/hw/riscv/virt.c b/hw/riscv/virt.c
index cfd52bc59b..1d05bb3ef9 100644
--- a/hw/riscv/virt.c
+++ b/hw/riscv/virt.c
@@ -449,7 +449,7 @@ static inline DeviceState *gpex_pcie_init(MemoryRegion
*sys_mem,
hwaddr ecam_base, hwaddr ecam_size,
hwaddr mmio_base, hwaddr mmio_size,
hwaddr pio_base,
- DeviceState *plic, bool link_up)
+ DeviceState *plic)
{
DeviceState *dev;
MemoryRegion *ecam_alias, *ecam_reg;
@@ -669,12 +669,12 @@ static void virt_machine_init(MachineState *machine)
}
gpex_pcie_init(system_memory,
- memmap[VIRT_PCIE_ECAM].base,
- memmap[VIRT_PCIE_ECAM].size,
- memmap[VIRT_PCIE_MMIO].base,
- memmap[VIRT_PCIE_MMIO].size,
- memmap[VIRT_PCIE_PIO].base,
- DEVICE(pcie_plic), true);
+ memmap[VIRT_PCIE_ECAM].base,
+ memmap[VIRT_PCIE_ECAM].size,
+ memmap[VIRT_PCIE_MMIO].base,
+ memmap[VIRT_PCIE_MMIO].size,
+ memmap[VIRT_PCIE_PIO].base,
+ DEVICE(pcie_plic));
serial_mm_init(system_memory, memmap[VIRT_UART0].base,
0, qdev_get_gpio_in(DEVICE(mmio_plic), UART0_IRQ), 399193,
--
2.25.1
[PATCH 3/4] hw/riscv: virt: Limit RAM size in a 32-bit system
From: Bin Meng
RV32 supports 34-bit physical address hence the maximum RAM size
should be limitted. Limit the RAM size to 10 GiB, which leaves
some room for PCIe high mmio space.
Signed-off-by: Bin Meng
---
hw/riscv/virt.c | 13 +
1 file changed, 13 insertions(+)
diff --git a/hw/riscv/virt.c b/hw/riscv/virt.c
index 1d05bb3ef9..4f44509360 100644
--- a/hw/riscv/virt.c
+++ b/hw/riscv/virt.c
@@ -590,6 +590,19 @@ static void virt_machine_init(MachineState *machine)
}
}
+/* limit RAM size in a 32-bit system */
+if (riscv_is_32bit(&s->soc[0])) {
+/*
+ * Cast machine->ram_size to 64-bit for 32-bit host,
+ * to make the build on 32-bit host happy.
+ */
+if ((uint64_t)(machine->ram_size) > 10 * GiB) {
+/* 32-bit host won't have a chance to execute here */
+machine->ram_size = 10 * GiB;
+error_report("Limitting RAM size to 10 GiB");
+}
+}
+
/* register system main memory (actual RAM) */
memory_region_init_ram(main_mem, NULL, "riscv_virt_board.ram",
machine->ram_size, &error_fatal);
--
2.25.1
[PATCH 4/4] hw/riscv: virt: Map high mmio for PCIe
From: Bin Meng
Some peripherals require 64-bit PCI address, so let's map the high
mmio space for PCIe.
For RV32, the address is hardcoded to below 4 GiB from the highest
accessible physical address. For RV64, the base address depends on
top of RAM and is aligned to its size which is using 16 GiB for now.
Signed-off-by: Bin Meng
---
hw/riscv/virt.c | 36 ++--
1 file changed, 34 insertions(+), 2 deletions(-)
diff --git a/hw/riscv/virt.c b/hw/riscv/virt.c
index 4f44509360..4ab3b35cc7 100644
--- a/hw/riscv/virt.c
+++ b/hw/riscv/virt.c
@@ -59,6 +59,15 @@ static const MemMapEntry virt_memmap[] = {
[VIRT_DRAM] ={ 0x8000, 0x0 },
};
+/* PCIe high mmio is fixed for RV32 */
+#define VIRT32_HIGH_PCIE_MMIO_BASE 0x3ULL
+#define VIRT32_HIGH_PCIE_MMIO_SIZE (4 * GiB)
+
+/* PCIe high mmio for RV64, size is fixed but base depends on top of RAM */
+#define VIRT64_HIGH_PCIE_MMIO_SIZE (16 * GiB)
+
+static MemMapEntry virt_high_pcie_memmap;
+
#define VIRT_FLASH_SECTOR_SIZE (256 * KiB)
static PFlashCFI01 *virt_flash_create1(RISCVVirtState *s,
@@ -371,7 +380,11 @@ static void create_fdt(RISCVVirtState *s, const
MemMapEntry *memmap,
2, memmap[VIRT_PCIE_PIO].base, 2, memmap[VIRT_PCIE_PIO].size,
1, FDT_PCI_RANGE_MMIO,
2, memmap[VIRT_PCIE_MMIO].base,
-2, memmap[VIRT_PCIE_MMIO].base, 2, memmap[VIRT_PCIE_MMIO].size);
+2, memmap[VIRT_PCIE_MMIO].base, 2, memmap[VIRT_PCIE_MMIO].size,
+1, FDT_PCI_RANGE_MMIO_64BIT,
+2, virt_high_pcie_memmap.base,
+2, virt_high_pcie_memmap.base, 2, virt_high_pcie_memmap.size);
+
create_pcie_irq_map(fdt, name, plic_pcie_phandle);
g_free(name);
@@ -448,12 +461,14 @@ update_bootargs:
static inline DeviceState *gpex_pcie_init(MemoryRegion *sys_mem,
hwaddr ecam_base, hwaddr ecam_size,
hwaddr mmio_base, hwaddr mmio_size,
+ hwaddr high_mmio_base,
+ hwaddr high_mmio_size,
hwaddr pio_base,
DeviceState *plic)
{
DeviceState *dev;
MemoryRegion *ecam_alias, *ecam_reg;
-MemoryRegion *mmio_alias, *mmio_reg;
+MemoryRegion *mmio_alias, *high_mmio_alias, *mmio_reg;
qemu_irq irq;
int i;
@@ -473,6 +488,13 @@ static inline DeviceState *gpex_pcie_init(MemoryRegion
*sys_mem,
mmio_reg, mmio_base, mmio_size);
memory_region_add_subregion(get_system_memory(), mmio_base, mmio_alias);
+/* Map high MMIO space */
+high_mmio_alias = g_new0(MemoryRegion, 1);
+memory_region_init_alias(high_mmio_alias, OBJECT(dev), "pcie-mmio-high",
+ mmio_reg, high_mmio_base, high_mmio_size);
+memory_region_add_subregion(get_system_memory(), high_mmio_base,
+high_mmio_alias);
+
sysbus_mmio_map(SYS_BUS_DEVICE(dev), 2, pio_base);
for (i = 0; i < GPEX_NUM_IRQS; i++) {
@@ -601,6 +623,14 @@ static void virt_machine_init(MachineState *machine)
machine->ram_size = 10 * GiB;
error_report("Limitting RAM size to 10 GiB");
}
+
+virt_high_pcie_memmap.base = VIRT32_HIGH_PCIE_MMIO_BASE;
+virt_high_pcie_memmap.size = VIRT32_HIGH_PCIE_MMIO_SIZE;
+} else {
+virt_high_pcie_memmap.size = VIRT64_HIGH_PCIE_MMIO_SIZE;
+virt_high_pcie_memmap.base = memmap[VIRT_DRAM].base +
machine->ram_size;
+virt_high_pcie_memmap.base =
+ROUND_UP(virt_high_pcie_memmap.base, virt_high_pcie_memmap.size);
}
/* register system main memory (actual RAM) */
@@ -686,6 +716,8 @@ static void virt_machine_init(MachineState *machine)
memmap[VIRT_PCIE_ECAM].size,
memmap[VIRT_PCIE_MMIO].base,
memmap[VIRT_PCIE_MMIO].size,
+ virt_high_pcie_memmap.base,
+ virt_high_pcie_memmap.size,
memmap[VIRT_PCIE_PIO].base,
DEVICE(pcie_plic));
--
2.25.1
Re: [PATCH v7 07/11] iotests: add findtests.py
Am 22.01.2021 um 12:57 hat Vladimir Sementsov-Ogievskiy geschrieben:
> 22.01.2021 14:48, Kevin Wolf wrote:
> > Am 16.01.2021 um 14:44 hat Vladimir Sementsov-Ogievskiy geschrieben:
> > > +def add_group_file(self, fname: str) -> None:
> > > +with open(fname) as f:
> > > +for line in f:
> > > +line = line.strip()
> > > +
> > > +if (not line) or line[0] == '#':
> > > +continue
> > > +
> > > +words = line.split()
> > > +test_file = self.parse_test_name(words[0])
> > > +groups = words[1:]
> >
> > The previous version still had this:
> >
> > +if test_file not in self.all_tests:
> > +print(f'Warning: {fname}: "{test_file}" test is not
> > found.'
> > + ' Skip.')
> > +continue
> >
> > Why did you remove it? I found this useful when I had a wrong test name
> > in my group.local. Now it's silently ignored.
>
>
> Because now we use parse_test_name which will raise ValueError, so we
> will not go to this if anyway.
>
> So, wrong name will not be silently ignored, check will fail, and
> you'll have to fix group file.. It is suitable?
It doesn't, though.
Oh, wait... Is it possible that you lost support for group.local
altogether? grep for "group.local" comes up empty, and add_group_file()
is only defined, but never called.
So the reason for the behaviour seems to be that it doesn't even try to
parse the group file.
Kevin
Re: [PULL 10/11] vnc: move initialization to framebuffer_update_request
On 01/22/21 09:46, Gerd Hoffmann wrote:
>> This patch breaks QEMU for me.
>
>> The symptom is the following: in virt-manager, the display remains dead
>> (black), when I start an OVMF guest. At the same time, unusually high
>> CPU load can be seen on the host; it makes me think that virt-manager is
>> trying, in a busy loop, to complete the VNC handshake, or some such.
>> Ultimately, although the guest boots up fine, virt-manager gives up, and
>> the display window says "Viewer was disconnected".
>
> It is the vnc_colordepth() call. Seems gtk-vnc sends a update request
> with incremental=0 as response to the VNC_ENCODING_WMVi message. So
> sending that as response to an incremental=0 update request creates an
> endless loop ...
Interesting; I saw that commit 9e1632ad07ca *added* (as opposed to
*moving*) the vnc_colordepth() call; I thought it was a relatively
insignificant bit...
I'll report back on your separately posted patch.
Thanks!
Laszlo
>
> take care,
> Gerd
>
> diff --git a/ui/vnc.c b/ui/vnc.c
> index d429bfee5a65..0a3e2f4aa98c 100644
> --- a/ui/vnc.c
> +++ b/ui/vnc.c
> @@ -2041,7 +2041,6 @@ static void framebuffer_update_request(VncState *vs,
> int incremental,
> } else {
> vs->update = VNC_STATE_UPDATE_FORCE;
> vnc_set_area_dirty(vs->dirty, vs->vd, x, y, w, h);
> -vnc_colordepth(vs);
> vnc_desktop_resize(vs);
> vnc_led_state_change(vs);
> vnc_cursor_define(vs);
>
Re: [PATCH v2] 9pfs: Improve unreclaim loop
On Donnerstag, 21. Januar 2021 19:15:10 CET Greg Kurz wrote:
> If a fid was actually re-opened by v9fs_reopen_fid(), we re-traverse the
> fid list from the head in case some other request created a fid that
> needs to be marked unreclaimable as well (ie. the client opened a new
That's "i.e.". Not a big deal so ...
Reviewed-by: Christian Schoenebeck
> handle on the path that is being unlinked). This is suboptimal since
> most if not all fids that require it have likely been taken care of
> already.
>
> This is mostly the result of new fids being added to the head of the
> list. Since the list is now a QSIMPLEQ, add new fids at the end instead
> to avoid the need to rewind. Take a reference on the fid to ensure it
> doesn't go away during v9fs_reopen_fid() and that it can be safely
> passed to QSIMPLEQ_NEXT() afterwards. Since the associated put_fid()
> can also yield, same is done with the next fid. So the logic here is
> to get a reference on a fid and only put it back during the next
> iteration after we could get a reference on the next fid.
>
> Signed-off-by: Greg Kurz
> ---
> v2: - fix typos in changelog
> - drop bogus assert()
> ---
> hw/9pfs/9p.c | 46 --
> 1 file changed, 32 insertions(+), 14 deletions(-)
>
> diff --git a/hw/9pfs/9p.c b/hw/9pfs/9p.c
> index b65f320e6518..3864d014b43c 100644
> --- a/hw/9pfs/9p.c
> +++ b/hw/9pfs/9p.c
> @@ -311,7 +311,7 @@ static V9fsFidState *alloc_fid(V9fsState *s, int32_t
> fid) * reclaim won't close the file descriptor
> */
> f->flags |= FID_REFERENCED;
> -QSIMPLEQ_INSERT_HEAD(&s->fid_list, f, next);
> +QSIMPLEQ_INSERT_TAIL(&s->fid_list, f, next);
>
> v9fs_readdir_init(s->proto_version, &f->fs.dir);
> v9fs_readdir_init(s->proto_version, &f->fs_reclaim.dir);
> @@ -497,32 +497,50 @@ static int coroutine_fn
> v9fs_mark_fids_unreclaim(V9fsPDU *pdu, V9fsPath *path) {
> int err;
> V9fsState *s = pdu->s;
> -V9fsFidState *fidp;
> +V9fsFidState *fidp, *fidp_next;
>
> -again:
> -QSIMPLEQ_FOREACH(fidp, &s->fid_list, next) {
> -if (fidp->path.size != path->size) {
> -continue;
> -}
> -if (!memcmp(fidp->path.data, path->data, path->size)) {
> +fidp = QSIMPLEQ_FIRST(&s->fid_list);
> +if (!fidp) {
> +return 0;
> +}
> +
> +/*
> + * v9fs_reopen_fid() can yield : a reference on the fid must be held
> + * to ensure its pointer remains valid and we can safely pass it to
> + * QSIMPLEQ_NEXT(). The corresponding put_fid() can also yield so
> + * we must keep a reference on the next fid as well. So the logic here
> + * is to get a reference on a fid and only put it back during the next
> + * iteration after we could get a reference on the next fid. Start with
> + * the first one.
> + */
> +for (fidp->ref++; fidp; fidp = fidp_next) {
> +if (fidp->path.size == path->size &&
> +!memcmp(fidp->path.data, path->data, path->size)) {
> /* Mark the fid non reclaimable. */
> fidp->flags |= FID_NON_RECLAIMABLE;
>
> /* reopen the file/dir if already closed */
> err = v9fs_reopen_fid(pdu, fidp);
> if (err < 0) {
> +put_fid(pdu, fidp);
> return err;
> }
> +}
> +
> +fidp_next = QSIMPLEQ_NEXT(fidp, next);
> +
> +if (fidp_next) {
> /*
> - * Go back to head of fid list because
> - * the list could have got updated when
> - * switched to the worker thread
> + * Ensure the next fid survives a potential clunk request
> during + * put_fid() below and v9fs_reopen_fid() in the next
> iteration. */
> -if (err == 0) {
> -goto again;
> -}
> +fidp_next->ref++;
> }
> +
> +/* We're done with this fid */
> +put_fid(pdu, fidp);
> }
> +
> return 0;
> }
Re: [PULL 0/5] Linux user for 6.0 patches
On Thu, 21 Jan 2021 at 12:43, Laurent Vivier wrote: > > The following changes since commit 48202c712412c803ddb56365c7bca322aa4e7506: > > Merge remote-tracking branch 'remotes/pmaydell/tags/pull-target-arm-2021011= > 9-1' into staging (2021-01-19 15:47:23 +) > > are available in the Git repository at: > > git://github.com/vivier/qemu.git tags/linux-user-for-6.0-pull-request > > for you to fetch changes up to b1d2e476e94cb215d9e19fef1049d413b414ffc2: > > linux-user: Remove obsolete F_SHLCK and F_EXLCK translation (2021-01-21 13:= > 27:34 +0100) > > > linux-user pull request 20210119-v3 > > Remove obsolete F_SHLCK and F_EXLCK translation > Update sockopt > Add F_ADD_SEALS and F_GET_SEALS Applied, thanks. Please update the changelog at https://wiki.qemu.org/ChangeLog/6.0 for any user-visible changes. -- PMM
Re: About 'qemu-security' list subscription process
+-- On Fri, 15 Jan 2021, Daniel P. Berrangé wrote --+ | IOW ideally there should be some web of trust whereby some existing | member(s) knows the person/entity who is requesting acces. Other cases would | have to be evaluated case-by-case basis. * True, sounds reasonable. I'll probably start a thread on the -sec list for pending requests. Thank you. -- Prasad J Pandit / Red Hat Product Security Team 8685 545E B54C 486B C6EB 271E E285 8B5A F050 DE8D
Re: [PATCH v7 07/11] iotests: add findtests.py
22.01.2021 15:45, Kevin Wolf wrote:
Am 22.01.2021 um 12:57 hat Vladimir Sementsov-Ogievskiy geschrieben:
22.01.2021 14:48, Kevin Wolf wrote:
Am 16.01.2021 um 14:44 hat Vladimir Sementsov-Ogievskiy geschrieben:
+def add_group_file(self, fname: str) -> None:
+with open(fname) as f:
+for line in f:
+line = line.strip()
+
+if (not line) or line[0] == '#':
+continue
+
+words = line.split()
+test_file = self.parse_test_name(words[0])
+groups = words[1:]
The previous version still had this:
+if test_file not in self.all_tests:
+print(f'Warning: {fname}: "{test_file}" test is not found.'
+ ' Skip.')
+continue
Why did you remove it? I found this useful when I had a wrong test name
in my group.local. Now it's silently ignored.
Because now we use parse_test_name which will raise ValueError, so we
will not go to this if anyway.
So, wrong name will not be silently ignored, check will fail, and
you'll have to fix group file.. It is suitable?
It doesn't, though.
Oh, wait... Is it possible that you lost support for group.local
altogether? grep for "group.local" comes up empty, and add_group_file()
is only defined, but never called.
So the reason for the behaviour seems to be that it doesn't even try to
parse the group file.
Ooops, you are right :( I've dropped an extra layer of indirection to make
things simpler and group.local was lost. It's the reason to send v8, I'll do it
now.
In a mean time, reverting 06 for now is OK for me.
--
Best regards,
Vladimir
Re: [PATCH] arm: rename xlnx-zcu102.canbusN properties
Just noticed this wasn't cc'd to the Xilinx folks. Would one
of you like to review it?
thanks
-- PMM
On Mon, 18 Jan 2021 at 16:25, Paolo Bonzini wrote:
>
> The properties to attach a CANBUS object to the xlnx-zcu102 machine have
> a period in them. We want to use periods in properties for compound QAPI
> types,
> and besides the "xlnx-zcu102." prefix is both unnecessary and different
> from any other machine property name. Remove it.
>
> Signed-off-by: Paolo Bonzini
> ---
> hw/arm/xlnx-zcu102.c| 4 ++--
> tests/qtest/xlnx-can-test.c | 30 +++---
> 2 files changed, 17 insertions(+), 17 deletions(-)
>
> diff --git a/hw/arm/xlnx-zcu102.c b/hw/arm/xlnx-zcu102.c
> index 4ef0c516bf..c9713638c5 100644
> --- a/hw/arm/xlnx-zcu102.c
> +++ b/hw/arm/xlnx-zcu102.c
> @@ -219,12 +219,12 @@ static void xlnx_zcu102_machine_instance_init(Object
> *obj)
> s->secure = false;
> /* Default to virt (EL2) being disabled */
> s->virt = false;
> -object_property_add_link(obj, "xlnx-zcu102.canbus0", TYPE_CAN_BUS,
> +object_property_add_link(obj, "canbus0", TYPE_CAN_BUS,
> (Object **)&s->canbus[0],
> object_property_allow_set_link,
> 0);
>
> -object_property_add_link(obj, "xlnx-zcu102.canbus1", TYPE_CAN_BUS,
> +object_property_add_link(obj, "canbus1", TYPE_CAN_BUS,
> (Object **)&s->canbus[1],
> object_property_allow_set_link,
> 0);
> diff --git a/tests/qtest/xlnx-can-test.c b/tests/qtest/xlnx-can-test.c
> index 3d1120005b..54de71a686 100644
> --- a/tests/qtest/xlnx-can-test.c
> +++ b/tests/qtest/xlnx-can-test.c
> @@ -138,9 +138,9 @@ static void test_can_bus(void)
> uint8_t can_timestamp = 1;
>
> QTestState *qts = qtest_init("-machine xlnx-zcu102"
> -" -object can-bus,id=canbus0"
> -" -machine xlnx-zcu102.canbus0=canbus0"
> -" -machine xlnx-zcu102.canbus1=canbus0"
> +" -object can-bus,id=canbus"
> +" -machine canbus0=canbus"
> +" -machine canbus1=canbus"
> );
>
> /* Configure the CAN0 and CAN1. */
> @@ -175,9 +175,9 @@ static void test_can_loopback(void)
> uint32_t status = 0;
>
> QTestState *qts = qtest_init("-machine xlnx-zcu102"
> -" -object can-bus,id=canbus0"
> -" -machine xlnx-zcu102.canbus0=canbus0"
> -" -machine xlnx-zcu102.canbus1=canbus0"
> +" -object can-bus,id=canbus"
> +" -machine canbus0=canbus"
> +" -machine canbus1=canbus"
> );
>
> /* Configure the CAN0 in loopback mode. */
> @@ -223,9 +223,9 @@ static void test_can_filter(void)
> uint8_t can_timestamp = 1;
>
> QTestState *qts = qtest_init("-machine xlnx-zcu102"
> -" -object can-bus,id=canbus0"
> -" -machine xlnx-zcu102.canbus0=canbus0"
> -" -machine xlnx-zcu102.canbus1=canbus0"
> +" -object can-bus,id=canbus"
> +" -machine canbus0=canbus"
> +" -machine canbus1=canbus"
> );
>
> /* Configure the CAN0 and CAN1. */
> @@ -271,9 +271,9 @@ static void test_can_sleepmode(void)
> uint8_t can_timestamp = 1;
>
> QTestState *qts = qtest_init("-machine xlnx-zcu102"
> -" -object can-bus,id=canbus0"
> -" -machine xlnx-zcu102.canbus0=canbus0"
> -" -machine xlnx-zcu102.canbus1=canbus0"
> +" -object can-bus,id=canbus"
> +" -machine canbus0=canbus"
> +" -machine canbus1=canbus"
> );
>
> /* Configure the CAN0. */
> @@ -317,9 +317,9 @@ static void test_can_snoopmode(void)
> uint8_t can_timestamp = 1;
>
> QTestState *qts = qtest_init("-machine xlnx-zcu102"
> -" -object can-bus,id=canbus0"
> -" -machine xlnx-zcu102.canbus0=canbus0"
> -" -machine xlnx-zcu102.canbus1=canbus0"
> +" -object can-bus,id=canbus"
> +" -machine canbus0=canbus"
> +" -machine canbus1=canbus"
> );
>
> /* Configure the CAN0. */
> --
> 2.26.2
[Bug 1912224] Re: qemu may freeze during drive-mirroring on fragmented FS
Seems a regression introduce in commit : (2.7.0)
commit 0965a41e998ab820b5d660c8abfc8c819c97bc1b
Author: Vladimir Sementsov-Ogievskiy
Date: Thu Jul 14 20:19:01 2016 +0300
mirror: double performance of the bulk stage if the disc is full
Mirror can do up to 16 in-flight requests, but actually on full copy
(the whole source disk is non-zero) in-flight is always 1. This happens
as the request is not limited in size: the data occupies maximum
available
capacity of s->buf.
The patch limits the size of the request to some artificial constant
(1 Mb here), which is not that big or small. This effectively enables
back parallelism in mirror code as it was designed.
The result is important: the time to migrate 10 Gb disk is reduced from
~350 sec to 170 sec.
If I revert this commit on master on top of (fef80ea073c48) (minor
conflicts) issue disappears.
on a fragmented file (average size per exent 2125 KB) of 58GB:
upstream 207s (285 MB/s) instance is broken during mirroring
upstream with revert 225s (262 MB/s) instance is fine.
not fully tested, but the patch probably improve perf in other cases
(as disccuss in git message), maybe just need to improve to avoid the freeze?
--
You received this bug notification because you are a member of qemu-
devel-ml, which is subscribed to QEMU.
https://bugs.launchpad.net/bugs/1912224
Title:
qemu may freeze during drive-mirroring on fragmented FS
Status in QEMU:
New
Bug description:
We have odd behavior in operation where qemu freeze during long
seconds, We started an thread about that issue here:
https://lists.gnu.org/archive/html/qemu-devel/2020-11/msg05623.html
It happens at least during openstack nova snapshot (qemu blockdev-mirror)
or live block migration(which include network copy of disk).
After further troubleshoots, it seems related to FS fragmentation on
host.
reproducible at least on:
Ubuntu 18.04.3/4.18.0-25-generic/qemu-4.0
Ubuntu 16.04.6/5.10.6/qemu-5.2.0-rc2
# Lets create a dedicated file system on a SSD/Nvme 60GB disk in my case:
$sudo mkfs.ext4 /dev/sda3
$sudo mount /dev/sda3 /mnt
$df -h /mnt
Filesystem Size Used Avail Use% Mounted on
/dev/sda3 59G 53M 56G 1% /mnt
#Create a fragmented disk on it using 2MB Chunks (about 30min):
$sudo python3 create_fragged_disk.py /mnt 2
Filling up FS by Creating chunks files in: /mnt/chunks
We are probably full as expected!!: [Errno 28] No space left on device
Creating fragged disk file: /mnt/disk
$ls -lhs
59G -rw-r--r-- 1 root root 59G Jan 15 14:08 /mnt/disk
$ sudo e4defrag -c /mnt/disk
Total/best extents 41971/30
Average size per extent1466 KB
Fragmentation score2
[0-30 no problem: 31-55 a little bit fragmented: 56- needs defrag]
This file (/mnt/disk) does not need defragmentation.
Done.
# the tool^^^ says it is not enough fragmented to be able to defrag.
#Inject an image on fragmented disk
sudo chown ubuntu /mnt/disk
wget
https://cloud-images.ubuntu.com/bionic/current/bionic-server-cloudimg-amd64.img
qemu-img convert -O raw bionic-server-cloudimg-amd64.img \
bionic-server-cloudimg-amd64.img.raw
dd conv=notrunc iflag=fullblock if=bionic-server-cloudimg-amd64.img.raw \
of=/mnt/disk bs=1M
virt-customize -a /mnt/disk --root-password password:
# logon run console activity ex: ping -i 0.3 127.0.0.1
$qemu-system-x86_64 -m 2G -enable-kvm -nographic \
-chardev socket,id=test,path=/tmp/qmp-monitor,server,nowait \
-mon chardev=test,mode=control \
-drive
file=/mnt/disk,format=raw,if=none,id=drive-virtio-disk0,cache=none,discard\
-device
virtio-blk-pci,scsi=off,drive=drive-virtio-disk0,id=virtio-disk0,bootindex=1,write-cache=on
$sync
$echo 3 | sudo tee -a /proc/sys/vm/drop_caches
#start drive-mirror via qmp on another SSD/nvme partition
nc -U /tmp/qmp-monitor
{"execute":"qmp_capabilities"}
{"execute":"drive-mirror","arguments":{"device":"drive-virtio-disk0","target":"/home/ubuntu/mirror","sync":"full","format":"qcow2"}}
^^^ qemu console may start to freeze at this step.
NOTE:
- smaller chunk sz and bigger disk size the worst it is.
In operation we also have issue on 400GB disk size with average 13MB/extent
- Reproducible also on xfs
Expected behavior:
---
QEMU should remain steady, eventually only have decrease storage Performance
or mirroring, because of fragmented fs.
Observed behavior:
---
Perf of mirroring is still quite good even on fragmented FS,
but it breaks qemu.
## create_fragged_disk.py
import sys
import os
import tempfile
import glob
import errno
MNT_DIR = sys.argv[1]
CHUNK_SZ_MB = int(sys.argv[2])
[PATCH v3 0/4] meson: Try to clarify TCG / TCI options for new users
Since v2: - Included Thomas suggestions Some new users get confused between 'TCG' and 'TCI' and enable TCI when TCG is better for they needs. Try to clarify it is better to not use TCI when native backend is available. Note, before Meson, warnings were summarized at the end of ./configure. Now they are displayed earlier, and likely missed IMHO. No clue how to improve that :/ Based-on: <20210121095616.1471869-1-phi...@redhat.com> Philippe Mathieu-Daudé (4): meson: Explicit TCG backend used meson: Warn when TCI is selected but TCG backend is available configure: Improve TCI feature description configure: Reword --enable-tcg-interpreter as --disable-native-tcg configure | 5 +++-- meson.build | 11 +-- 2 files changed, 12 insertions(+), 4 deletions(-) -- 2.26.2
[PATCH v3 2/4] meson: Warn when TCI is selected but TCG backend is available
Some new users get confused with 'TCG' and 'TCI', and enable TCI
support expecting to enable TCG.
Emit a warning when native TCG backend is available on the
host architecture, mentioning this is a suboptimal configuration.
Reviewed-by: Stefan Weil
Reviewed-by: Thomas Huth
Signed-off-by: Philippe Mathieu-Daudé
---
meson.build | 3 +++
1 file changed, 3 insertions(+)
diff --git a/meson.build b/meson.build
index d5b76150e49..d3df5fa3516 100644
--- a/meson.build
+++ b/meson.build
@@ -234,6 +234,9 @@
error('Unsupported CPU @0@, try --enable-tcg-interpreter'.format(cpu))
endif
endif
+ if 'CONFIG_TCG_INTERPRETER' in config_host and cpu in supported_cpus
+warning('Experimental TCI requested while native TCG is available on @0@,
suboptimal performance expected'.format(cpu))
+ endif
accelerators += 'CONFIG_TCG'
config_host += { 'CONFIG_TCG': 'y' }
endif
--
2.26.2
[PATCH v3 1/4] meson: Explicit TCG backend used
Reviewed-by: Thomas Huth
Reviewed-by: Stefan Weil
Signed-off-by: Philippe Mathieu-Daudé
---
meson.build | 8 ++--
1 file changed, 6 insertions(+), 2 deletions(-)
diff --git a/meson.build b/meson.build
index 8535a83fb70..d5b76150e49 100644
--- a/meson.build
+++ b/meson.build
@@ -229,7 +229,7 @@
if not get_option('tcg').disabled()
if cpu not in supported_cpus
if 'CONFIG_TCG_INTERPRETER' in config_host
- warning('Unsupported CPU @0@, will use TCG with TCI
(experimental)'.format(cpu))
+ warning('Unsupported CPU @0@, will use TCG with TCI (experimental and
slow)'.format(cpu))
else
error('Unsupported CPU @0@, try --enable-tcg-interpreter'.format(cpu))
endif
@@ -2419,8 +2419,12 @@
endif
summary_info += {'TCG support': config_all.has_key('CONFIG_TCG')}
if config_all.has_key('CONFIG_TCG')
+ if config_host.has_key('CONFIG_TCG_INTERPRETER')
+summary_info += {'TCG backend': 'TCG interpreter (experimental)'}
+ else
+summary_info += {'TCG backend': 'native (@0@)'.format(cpu)}
+ endif
summary_info += {'TCG debug enabled':
config_host.has_key('CONFIG_DEBUG_TCG')}
- summary_info += {'TCG interpreter':
config_host.has_key('CONFIG_TCG_INTERPRETER')}
endif
summary_info += {'target list': ' '.join(target_dirs)}
if have_system
--
2.26.2
[PATCH v3 3/4] configure: Improve TCI feature description
Users might want to enable all features, without realizing some features have negative effect. Mention the TCI feature is slow and experimental, hoping it will be selected knowingly. Suggested-by: Thomas Huth Signed-off-by: Philippe Mathieu-Daudé --- configure | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/configure b/configure index 9f016b06b54..48bd6f48d7a 100755 --- a/configure +++ b/configure @@ -1753,7 +1753,7 @@ Advanced options (experts only): --with-trace-file=NAME Full PATH,NAME of file to store traces Default:trace- --disable-slirp disable SLIRP userspace network connectivity - --enable-tcg-interpreter enable TCG with bytecode interpreter (TCI) + --enable-tcg-interpreter enable TCI (TCG with bytecode interpreter, experimental and slow) --enable-malloc-trim enable libc malloc_trim() for memory optimization --oss-libpath to OSS library --cpu=CPUBuild for host CPU [$cpu] -- 2.26.2
Re: [PATCH v7 07/11] iotests: add findtests.py
Am 22.01.2021 um 14:16 hat Vladimir Sementsov-Ogievskiy geschrieben:
> 22.01.2021 15:45, Kevin Wolf wrote:
> > Am 22.01.2021 um 12:57 hat Vladimir Sementsov-Ogievskiy geschrieben:
> > > 22.01.2021 14:48, Kevin Wolf wrote:
> > > > Am 16.01.2021 um 14:44 hat Vladimir Sementsov-Ogievskiy geschrieben:
> > > > > +def add_group_file(self, fname: str) -> None:
> > > > > +with open(fname) as f:
> > > > > +for line in f:
> > > > > +line = line.strip()
> > > > > +
> > > > > +if (not line) or line[0] == '#':
> > > > > +continue
> > > > > +
> > > > > +words = line.split()
> > > > > +test_file = self.parse_test_name(words[0])
> > > > > +groups = words[1:]
> > > >
> > > > The previous version still had this:
> > > >
> > > > +if test_file not in self.all_tests:
> > > > +print(f'Warning: {fname}: "{test_file}" test is
> > > > not found.'
> > > > + ' Skip.')
> > > > +continue
> > > >
> > > > Why did you remove it? I found this useful when I had a wrong test name
> > > > in my group.local. Now it's silently ignored.
> > >
> > >
> > > Because now we use parse_test_name which will raise ValueError, so we
> > > will not go to this if anyway.
> > >
> > > So, wrong name will not be silently ignored, check will fail, and
> > > you'll have to fix group file.. It is suitable?
> >
> > It doesn't, though.
> >
> > Oh, wait... Is it possible that you lost support for group.local
> > altogether? grep for "group.local" comes up empty, and add_group_file()
> > is only defined, but never called.
> >
> > So the reason for the behaviour seems to be that it doesn't even try to
> > parse the group file.
>
> Ooops, you are right :( I've dropped an extra layer of indirection to
> make things simpler and group.local was lost. It's the reason to send
> v8, I'll do it now.
You can wait with sending v8 until I've completed review in case
something else comes up. So far I'm done with the changes to the part
that I reviewed last time and apart from this bug they look good to me.
Now it's the remaining patches.
> In a mean time, reverting 06 for now is OK for me.
Not a big deal if we get it fixed soon, it only becomes a problem if the
rest of this series gets shelved for a longer time. Maybe we can
complete it today, maybe on Monday, and then I'll send a pull request
right away.
Kevin
[PATCH v3 4/4] configure: Reword --enable-tcg-interpreter as --disable-native-tcg
Users might want to enable all features, without realizing some features have negative effect. Rename '--enable-tcg-interpreter' as '--disable-native-tcg' to avoid user selecting this feature without understanding it. '--enable-tcg-interpreter' is kept in for backward compability with scripts. Suggested-by: Thomas Huth Reviewed-by: Thomas Huth Signed-off-by: Philippe Mathieu-Daudé --- configure | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/configure b/configure index 48bd6f48d7a..5e5ff779a69 100755 --- a/configure +++ b/configure @@ -1121,7 +1121,8 @@ for opt do ;; --disable-tcg-interpreter) tcg_interpreter="no" ;; - --enable-tcg-interpreter) tcg_interpreter="yes" + --enable-tcg-interpreter) # backward compatibility + --disable-native-tcg) tcg_interpreter="yes" ;; --disable-cap-ng) cap_ng="disabled" ;; @@ -1753,7 +1754,7 @@ Advanced options (experts only): --with-trace-file=NAME Full PATH,NAME of file to store traces Default:trace- --disable-slirp disable SLIRP userspace network connectivity - --enable-tcg-interpreter enable TCI (TCG with bytecode interpreter, experimental and slow) + --disable-native-tcg enable TCI (TCG with bytecode interpreter, experimental and slow) --enable-malloc-trim enable libc malloc_trim() for memory optimization --oss-libpath to OSS library --cpu=CPUBuild for host CPU [$cpu] -- 2.26.2
Re: [PATCH v8 00/10] hw/ssi: imx_spi: Fix various bugs in the imx_spi model
On Tue, Jan 19, 2021 at 9:40 PM Bin Meng wrote: > > From: Bin Meng > > This v8 series is based on the following 2 versions: > > - v5 series sent from Bin > http://patchwork.ozlabs.org/project/qemu-devel/list/?series=223919 > - v7 series sent from Philippe > http://patchwork.ozlabs.org/project/qemu-devel/list/?series=224612 > > This series fixes a bunch of bugs in current implementation of the imx > spi controller, including the following issues: > > - remove imx_spi_update_irq() in imx_spi_reset() > - chip select signal was not lower down when spi controller is disabled > - round up the tx burst length to be multiple of 8 > - transfer incorrect data when the burst length is larger than 32 bit > - spi controller tx and rx fifo endianness is incorrect > - remove pointless variable (s->burst_length) initialization (Philippe) > - rework imx_spi_reset() to keep CONREG register value (Philippe) > - rework imx_spi_read() to handle block disabled (Philippe) > - rework imx_spi_write() to handle block disabled (Philippe) > > Tested with upstream U-Boot v2020.10 (polling mode) and VxWorks 7 > (interrupt mode). > > Changes in v8: > - keep the controller disable logic in the ECSPI_CONREG case > in imx_spi_write() Ping?
Re: [PATCH] hw/mips: loongson3: Drop 'struct MemmapEntry'
On 1/22/21 1:24 PM, Bin Meng wrote: > From: Bin Meng > > There is already a MemMapEntry type defined in hwaddr.h. Let's drop > the loongson3 defined `struct MemmapEntry` and use the existing one. Not really... based on 0e324626306: $ git grep MemmapEntry origin/master -- include $ What tree are you using? Thanks, Phil.
Re: [PULL 10/11] vnc: move initialization to framebuffer_update_request
On Fri, Jan 22, 2021 at 01:49:38PM +0100, Laszlo Ersek wrote: > On 01/22/21 09:46, Gerd Hoffmann wrote: > >> This patch breaks QEMU for me. > > > >> The symptom is the following: in virt-manager, the display remains dead > >> (black), when I start an OVMF guest. At the same time, unusually high > >> CPU load can be seen on the host; it makes me think that virt-manager is > >> trying, in a busy loop, to complete the VNC handshake, or some such. > >> Ultimately, although the guest boots up fine, virt-manager gives up, and > >> the display window says "Viewer was disconnected". > > > > It is the vnc_colordepth() call. Seems gtk-vnc sends a update request > > with incremental=0 as response to the VNC_ENCODING_WMVi message. So > > sending that as response to an incremental=0 update request creates an > > endless loop ... > > Interesting; I saw that commit 9e1632ad07ca *added* (as opposed to > *moving*) the vnc_colordepth() call; I thought it was a relatively > insignificant bit... /me too. Some discussions in the resize changes indicated that the idea of a non-incremetal update request is that the server sends the *full* server-side state, so the client can render the screen properly without remembering old state. So I thought ok, lets send the colordepth info too, no big deal ... take care, Gerd
Re: [PATCH] hw/mips: loongson3: Drop 'struct MemmapEntry'
On Fri, Jan 22, 2021 at 9:37 PM Philippe Mathieu-Daudé wrote: > > On 1/22/21 1:24 PM, Bin Meng wrote: > > From: Bin Meng > > > > There is already a MemMapEntry type defined in hwaddr.h. Let's drop > > the loongson3 defined `struct MemmapEntry` and use the existing one. > > Not really... based on 0e324626306: > > $ git grep MemmapEntry origin/master -- include > $ > > What tree are you using? The latest origin/master Use $ git grep MemmapEntry origin/master -- It's not defined in include. Regards, Bin
Re: [PATCH] hw/mips: loongson3: Drop 'struct MemmapEntry'
On 1/22/21 2:37 PM, Philippe Mathieu-Daudé wrote: > On 1/22/21 1:24 PM, Bin Meng wrote: >> From: Bin Meng >> >> There is already a MemMapEntry type defined in hwaddr.h. Let's drop >> the loongson3 defined `struct MemmapEntry` and use the existing one. > > Not really... based on 0e324626306: > > $ git grep MemmapEntry origin/master -- include > $ OK I understood, you replaced MemmapEntry by MemMapEntry which is written slightly differently. Reviewed-by: Philippe Mathieu-Daudé Tested-by: Philippe Mathieu-Daudé Thanks!
Re: [PATCH 05/25] keyval: simplify keyval_parse_one
Paolo Bonzini writes:
> Now that the key is NULL terminated, we can remove some of the contortions
> that were done to operate on possibly '='-terminated strings in
> keyval_parse_one.
>
> Signed-off-by: Paolo Bonzini
Alright, I'm now ready to discuss the third argument: nicer code.
I think there is improvement, and I think it comes mostly from working
on a copy. Could be done without the syntax change.
I feel further improvement is possible, but that's no reason to delay
this series.
Second argument: better error messages. We discussed that in review of
a prior post, but you've since improved your error reporting further, so
let's have another look. I'm testing with
$ qemu-storage-daemon --nbd $ARG
because that one doesn't have an implied key, which permits slightly
simpler $ARG.
* Empty key
--nbd ,
master: Invalid parameter ''
your patch: Expected parameter before ','
Improvement.
--nbd key=val,=,fob=
master: Invalid parameter ''
your patch: Expected parameter before '=,fob='
Improvement.
* Empty key fragment
--nbd key..=
master: Invalid parameter 'key..'
your patch: same
Could use some love. Not your patch's fault.
--nbd .key=
master: Invalid parameter '..key'
your patch: same
Likweise.
If I omit the '=', your patch's message changes to
No implicit parameter name for value 'key..'
I consider that worse than before, because it's talking about
something outside the user's control (lack of an implict parameter
name) where it should instead tell the user what needs fixing in the
input.
* Missing value
--nbd key
master: Expected '=' after parameter 'key'
your patch: No implicit parameter name for value 'key'
Same criticism as above.
* Invalid key fragment
--nbd _=
master: Invalid parameter '_'
your patch: same
--nbd key.1a.b=
master: Invalid parameter 'key.1a.b'
your patch: same
Could perhaps use some love. Not your patch's fault.
--ndb anti,,social,,key=
master: Expected '=' after parameter 'anti'
your patch: Invalid parameter 'anti,social,key'
The new error message shows the *unescaped* string. Okay.
Your patch also adds an "Expected parameter at end of string" error.
Can you tell me how to trigger it?
I think there is improvement, and I think it could also be done without
the syntax change.
There also is one regression: "No implicit parameter name..." is no
good. Looks fixable to me.
I feel further improvement is possible, but that's no reason to delay
this series.
Now let me put the three arguments together.
Nicer code and better error reporting could be had with or without the
syntax change. With is a bit easier, because we already have your
patch.
The syntax change is a choice we can make. I'm reluctant to mess with
the syntax, but if you want the change, I'm not going to block it.
Hmm, bartering opportunity... May I have your support for me
eliminating anti-social device names in exchange? ;)
I believe your grammar is ambiguous. Your code seems to pick the sane
alternative. If I'm wrong, you need to enlighten me. If I'm right, you
need to fix your grammar.
> ---
> util/keyval.c | 27 ++-
> 1 file changed, 10 insertions(+), 17 deletions(-)
>
> diff --git a/util/keyval.c b/util/keyval.c
> index eb9b9c55ec..e7f708cd1e 100644
> --- a/util/keyval.c
> +++ b/util/keyval.c
> @@ -170,11 +170,10 @@ static QObject *keyval_parse_put(QDict *cur,
> *
> * On return:
> * - either NUL or the separator (comma or equal sign) is returned.
> - * - the length of the string is stored in @len.
> * - @start is advanced to either the NUL or the first character past the
> * separator.
> */
> -static char keyval_fetch_string(char **start, size_t *len, bool key)
> +static char keyval_fetch_string(char **start, bool key)
> {
> char sep;
> char *p, *unescaped;
> @@ -197,7 +196,6 @@ static char keyval_fetch_string(char **start, size_t
> *len, bool key)
> }
>
> *unescaped = 0;
> -*len = unescaped - *start;
> *start = p;
> return sep;
> }
> @@ -219,7 +217,7 @@ static char *keyval_parse_one(QDict *qdict, char *params,
>const char *implied_key, bool *help,
>Error **errp)
> {
> -const char *key, *key_end, *s, *end;
> +const char *key, *s, *end;
> const char *val = NULL;
> char sep;
> size_t len;
> @@ -229,8 +227,8 @@ static char *keyval_parse_one(QDict *qdict, char *params,
> QObject *next;
>
> key = params;
> -sep = keyval_fetch_string(¶ms, &len, true);
> -if (!len) {
> +sep = keyval_fetch_string(¶ms, true);
> +if (!*key) {
> if (sep) {
> error_setg(errp, "Expected parameter before '%c%s'", sep,
> params);
> } else {
> @@ -247,13 +245,11 @@ static char
Re: [PATCH v7 07/11] iotests: add findtests.py
22.01.2021 16:34, Kevin Wolf wrote:
Am 22.01.2021 um 14:16 hat Vladimir Sementsov-Ogievskiy geschrieben:
22.01.2021 15:45, Kevin Wolf wrote:
Am 22.01.2021 um 12:57 hat Vladimir Sementsov-Ogievskiy geschrieben:
22.01.2021 14:48, Kevin Wolf wrote:
Am 16.01.2021 um 14:44 hat Vladimir Sementsov-Ogievskiy geschrieben:
+def add_group_file(self, fname: str) -> None:
+with open(fname) as f:
+for line in f:
+line = line.strip()
+
+if (not line) or line[0] == '#':
+continue
+
+words = line.split()
+test_file = self.parse_test_name(words[0])
+groups = words[1:]
The previous version still had this:
+if test_file not in self.all_tests:
+print(f'Warning: {fname}: "{test_file}" test is not found.'
+ ' Skip.')
+continue
Why did you remove it? I found this useful when I had a wrong test name
in my group.local. Now it's silently ignored.
Because now we use parse_test_name which will raise ValueError, so we
will not go to this if anyway.
So, wrong name will not be silently ignored, check will fail, and
you'll have to fix group file.. It is suitable?
It doesn't, though.
Oh, wait... Is it possible that you lost support for group.local
altogether? grep for "group.local" comes up empty, and add_group_file()
is only defined, but never called.
So the reason for the behaviour seems to be that it doesn't even try to
parse the group file.
Ooops, you are right :( I've dropped an extra layer of indirection to
make things simpler and group.local was lost. It's the reason to send
v8, I'll do it now.
You can wait with sending v8 until I've completed review in case
something else comes up. So far I'm done with the changes to the part
that I reviewed last time and apart from this bug they look good to me.
Now it's the remaining patches.
OK. This thing is to be fixed in [10], not here, I'll send a squash-in
In a mean time, reverting 06 for now is OK for me.
Not a big deal if we get it fixed soon, it only becomes a problem if the
rest of this series gets shelved for a longer time. Maybe we can
complete it today, maybe on Monday, and then I'll send a pull request
right away.
Kevin
--
Best regards,
Vladimir
Re: [PATCH v7 10/11] iotests: rewrite check into python
16.01.2021 16:44, Vladimir Sementsov-Ogievskiy wrote:
Just use classes introduced in previous three commits. Behavior
difference is described in these three commits.
Drop group file, as it becomes unused.
Drop common.env: now check is in python, and for tests we use same
python interpreter that runs the check itself. Use build environment
PYTHON in check-block instead, to keep "make check" use the same
python.
Signed-off-by: Vladimir Sementsov-Ogievskiy
squash-in to support group.local:
@@ -117,6 +117,13 @@ if __name__ == '__main__':
groups = args.groups.split(',') if args.groups else None
x_groups = args.exlude_groups.split(',') if args.exclude_groups else None
+group_local = os.path.join(env.source_iotests, 'group.local')
+if os.path.isfile(group_local):
+try:
+testfinder.add_group_file(group_local)
+except ValueError as e:
+sys.exit(f"Filed to parse group file '{group_local}': {e}")
+
try:
tests = testfinder.find_tests(groups=groups, exclude_groups=x_groups,
tests=args.tests,
--
Best regards,
Vladimir
Re: [PATCH] vnc: drop vnc_colordepth() call.
Hi Gerd,
On 01/22/21 09:55, Gerd Hoffmann wrote:
> With gtk-vnc (which supports VNC_FEATURE_WMVI) this results in
> sending a VNC_ENCODING_WMVi message to the client. Which in turn
> seems to make gtk-vnc respond with another non-incremental update
> request. Hello endless loop ...
>
> Drop the call.
>
> Fixes: 9e1632ad07ca ("vnc: move initialization to framebuffer_update_request")
> Reported-by: Laszlo Ersek
> Signed-off-by: Gerd Hoffmann
> ---
> ui/vnc.c | 1 -
> 1 file changed, 1 deletion(-)
>
> diff --git a/ui/vnc.c b/ui/vnc.c
> index d429bfee5a65..0a3e2f4aa98c 100644
> --- a/ui/vnc.c
> +++ b/ui/vnc.c
> @@ -2041,7 +2041,6 @@ static void framebuffer_update_request(VncState *vs,
> int incremental,
> } else {
> vs->update = VNC_STATE_UPDATE_FORCE;
> vnc_set_area_dirty(vs->dirty, vs->vd, x, y, w, h);
> -vnc_colordepth(vs);
> vnc_desktop_resize(vs);
> vnc_led_state_change(vs);
> vnc_cursor_define(vs);
>
while this patch has some effect, it does not fix the issue.
* With the gvncviewer binary I mentioned before, there is no change in
behavior -- initial screen shown, no updates then, and finally
connection dropped:
> Connected to server
> Remote desktop size changed to 800x480
> Connection initialized
> Error: Failed to flush data
> Disconnected from server
* With virt-manager, there is a before-after difference: the screen is
now *flashing*, between actual content and a black void. Meanwhile the
VNC client is still spinning.
* If I pass "--gtk-vnc-debug" to "gvncviewer", the following log snippet
keeps repeating:
> src/vncconnection.c Emit main context 8
> src/vncconnection.c Re-requesting framebuffer update at 0,0 size 640x480,
> incremental 0
> src/vncconnection.c Num rects 1
> src/vncconnection.c FramebufferUpdate type=-223 area (640x480) at location 0,0
> src/vncconnection.c Desktop resize w=640 h=480
> src/vncconnection.c Emit main context 5
> src/vncdisplay.c Framebuffer size / format unchanged, skipping recreate
> src/vncconnection.c Requesting framebuffer update at 0,0 size 640x480,
> incremental 0
> src/vncconnection.c Num rects 1
> src/vncconnection.c FramebufferUpdate type=-261 area (1x1) at location 0,0
> src/vncconnection.c LED state: 0
Thanks
Laszlo
Re: [Qemu-devel] [PATCH v2 07/11] chardev: Let IOReadHandler use unsigned type
Hi Prasad, Richard. On 1/22/21 12:52 PM, P J P wrote: > +-- On Fri, 22 Jan 2021, Richard Purdie wrote --+ > | If so can anyone point me at that change? > | > | I ask since CVE-2018-18438 is marked as affecting all qemu versions > | (https://nvd.nist.gov/vuln/detail/CVE-2018-18438). > | > | If it was fixed, the version mask could be updated. If the fix wasn't > deemed > | worthwhile for some reason that is also fine and I can mark this one as > such > | in our system. I'm being told we only need one of the patches in this > series > | which I also don't believe as I suspect we either need the set or none of > | them! > | > | Any info would be most welcome. > > -> https://lists.gnu.org/archive/html/qemu-devel/2018-10/msg02239.html > -> https://lists.gnu.org/archive/html/qemu-devel/2018-10/msg02231.html > > * Yes, the type change fix had come up during patch reviews above, and this > series implemented the change. > > * Series is required IIUC, didn't realise it's not merged. Audit from Marc-André pointed that this is unlikely, we asked the reporter for a reproducer and got not news, and eventually closed this as NOTABUG (not even WONTFIX): https://bugzilla.redhat.com/show_bug.cgi?id=1609015 Regards, Phil.
Re: [PATCH v3 18/21] linux-user/aarch64: Signal SEGV_MTEAERR for async tag check error
On Fri, 15 Jan 2021 at 22:47, Richard Henderson
wrote:
>
> Signed-off-by: Richard Henderson
So when does the real kernel report async MTE exceptions to userspace?
The commit message would be a good place to briefly describe the
kernel's strategy and where QEMU differs from it (if anywhere)...
> ---
> linux-user/aarch64/target_signal.h | 1 +
> linux-user/aarch64/cpu_loop.c | 34 +-
> target/arm/mte_helper.c| 10 +
> 3 files changed, 35 insertions(+), 10 deletions(-)
>
> diff --git a/linux-user/aarch64/target_signal.h
> b/linux-user/aarch64/target_signal.h
> index 777fb667fe..18013e1b23 100644
> --- a/linux-user/aarch64/target_signal.h
> +++ b/linux-user/aarch64/target_signal.h
> @@ -21,6 +21,7 @@ typedef struct target_sigaltstack {
>
> #include "../generic/signal.h"
>
> +#define TARGET_SEGV_MTEAERR 8 /* Asynchronous ARM MTE error */
> #define TARGET_SEGV_MTESERR 9 /* Synchronous ARM MTE exception */
>
> #define TARGET_ARCH_HAS_SETUP_FRAME
> diff --git a/linux-user/aarch64/cpu_loop.c b/linux-user/aarch64/cpu_loop.c
> index 6867f0db2b..6160a401bd 100644
> --- a/linux-user/aarch64/cpu_loop.c
> +++ b/linux-user/aarch64/cpu_loop.c
> @@ -72,6 +72,21 @@
> put_user_u16(__x, (gaddr)); \
> })
>
> +static bool check_mte_async_fault(CPUARMState *env, target_siginfo_t *info)
> +{
> +if (likely(env->cp15.tfsr_el[0] == 0)) {
> +return false;
> +}
> +
> +env->cp15.tfsr_el[0] = 0;
> +info->si_signo = TARGET_SIGSEGV;
> +info->si_errno = 0;
> +info->_sifields._sigfault._addr = 0;
> +info->si_code = TARGET_SEGV_MTEAERR;
> +queue_signal(env, info->si_signo, QEMU_SI_FAULT, info);
> +return true;
> +}
> +
> /* AArch64 main loop */
> void cpu_loop(CPUARMState *env)
> {
> @@ -88,15 +103,13 @@ void cpu_loop(CPUARMState *env)
>
> switch (trapnr) {
> case EXCP_SWI:
> -ret = do_syscall(env,
> - env->xregs[8],
> - env->xregs[0],
> - env->xregs[1],
> - env->xregs[2],
> - env->xregs[3],
> - env->xregs[4],
> - env->xregs[5],
> - 0, 0);
> +if (check_mte_async_fault(env, &info)) {
> +ret = -TARGET_ERESTARTSYS;
> +} else {
> +ret = do_syscall(env, env->xregs[8], env->xregs[0],
> + env->xregs[1], env->xregs[2], env->xregs[3],
> + env->xregs[4], env->xregs[5], 0, 0);
> +}
> if (ret == -TARGET_ERESTARTSYS) {
> env->pc -= 4;
> } else if (ret != -TARGET_QEMU_ESIGRETURN) {
> @@ -104,7 +117,8 @@ void cpu_loop(CPUARMState *env)
> }
> break;
> case EXCP_INTERRUPT:
> -/* just indicate that signals should be handled asap */
> +/* Just indicate that signals should be handled asap. */
> +check_mte_async_fault(env, &info);
> break;
> case EXCP_UDEF:
> info.si_signo = TARGET_SIGILL;
So this doesn't guarantee to check the async-fault status on
every exit from cpu_exec(), which means we might miss things.
For instance I think this slightly contrived example would not
ever take the SEGV:
STR x0, [x1] # with a bad tag
YIELD
l: B l
because the STR and YIELD go into the same TB, the YIELD causes us
to leave the TB with EXCP_YIELD, we don't check for an async fault
in that code path, and then we'll go into the infinite loop and
have nothing to prompt us to come out and look at the async fault flags.
Does it work if we just always queue the SEGV on exit from cpu_exec()
and let the signal handling machinery prioritize if we also pend
some other signal because this was an EXCP_UDEF or whatever?
It would be neater if we could keep the fault-check outside the
switch (trapnr) somehow.
> diff --git a/target/arm/mte_helper.c b/target/arm/mte_helper.c
> index 153bd1e9df..d55f8d1e1e 100644
> --- a/target/arm/mte_helper.c
> +++ b/target/arm/mte_helper.c
> @@ -565,6 +565,16 @@ static void mte_check_fail(CPUARMState *env, uint32_t
> desc,
> select = 0;
> }
> env->cp15.tfsr_el[el] |= 1 << select;
> +#ifdef CONFIG_USER_ONLY
> +/*
> + * Stand in for a timer irq, setting _TIF_MTE_ASYNC_FAULT,
> + * which then sends a SIGSEGV when the thread is next scheduled.
> + * This cpu will return to the main loop at the end of the TB,
> + * which is rather sooner than "normal". But the alternative
> + * is waiting until the next syscall.
> + */
> +qemu_cpu_kick(env_cpu(env));
> +#endif
> break;
This does the right thing, but qemu_cpu_kick() is one of those
functions that's in a category of "not used m
Re: [PATCH v3 20/21] target/arm: Enable MTE for user-only
On Fri, 15 Jan 2021 at 22:47, Richard Henderson
wrote:
>
> Signed-off-by: Richard Henderson
> ---
> target/arm/cpu.c | 16
> 1 file changed, 16 insertions(+)
>
> diff --git a/target/arm/cpu.c b/target/arm/cpu.c
> index abc0affd00..5e613a747a 100644
> --- a/target/arm/cpu.c
> +++ b/target/arm/cpu.c
> @@ -208,6 +208,22 @@ static void arm_cpu_reset(DeviceState *dev)
> * Do not modify this without other changes.
> */
> env->cp15.tcr_el[1].raw_tcr = (3ULL << 37);
> +
> +/* Enable MTE */
> +if (cpu_isar_feature(aa64_mte, cpu)) {
> +/* Enable tag access, but leave TCF0 as No Effect (0). */
> +env->cp15.sctlr_el[1] |= SCTLR_ATA0;
> +/*
> + * Exclude all tags, so that tag 0 is always used.
> + * This corresponds to Linux current->thread.gcr_incl = 0.
> + *
> + * Set RRND, so that helper_irg() will generate a seed later.
> + * Here in cpu_reset(), the crypto subsystem has not yet been
> + * initialized.
> + */
> +env->cp15.gcr_el1 = 0x1;
> +}
> +
> # ifdef TARGET_TAGGED_ADDRESSES
> env->untagged_addr_mask = -1;
> # endif
> --
Reviewed-by: Peter Maydell
thanks
-- PMM
