Re: [RFC 0/3] aio-posix: call ->poll_end() when removing AioHandler
On 1/3/24 12:40, Fiona Ebner wrote: I'm happy to report that I cannot reproduce the CPU-usage-spike issue with the patch, but I did run into an assertion failure when trying to verify that it fixes my original stuck-guest-IO issue. See below for the backtrace [0]. Hanna wrote in https://issues.redhat.com/browse/RHEL-3934 I think it’s sufficient to simply call virtio_queue_notify_vq(vq) after the virtio_queue_aio_attach_host_notifier(vq, ctx) call, because both virtio-scsi’s and virtio-blk’s .handle_output() implementations acquire the device’s context, so this should be directly callable from any context. I guess this is not true anymore now that the AioContext locking was removed? Good point and, in fact, even before it was much safer to use virtio_queue_notify() instead. Not only does it use the event notifier handler, but it also calls it in the right thread/AioContext just by doing event_notifier_set(). The call to virtio_queue_set_notification(..., 1) is safe; not sure about the call to virtio_queue_set_notification(..., 0) though. I'd rather have that executed synchronously in the AioContext using aio_wait_bh_oneshot(). This is consistent with the pattern used by virtio_scsi_dataplane_stop() and virtio_blk_data_plane_stop(). Paolo
Re: [PATCH v2 12/12] hw/isa/vt82c686: Implement relocation and toggling of SuperI/O functions
On Tue, 2 Jan 2024, Bernhard Beschow wrote:
Am 24. Dezember 2023 00:51:53 UTC schrieb BALATON Zoltan :
On Tue, 19 Dec 2023, Bernhard Beschow wrote:
Am 19. Dezember 2023 00:26:15 UTC schrieb BALATON Zoltan :
On Mon, 18 Dec 2023, Bernhard Beschow wrote:
The VIA south bridges are able to relocate and toggle (enable or disable) their
SuperI/O functions. So far this is hardcoded such that all functions are always
enabled and are located at fixed addresses.
Some PC BIOSes seem to probe for I/O occupancy before activating such a function
and issue an error in case of a conflict. Since the functions are enabled on
reset, conflicts are always detected. Prevent that by implementing relocation
and toggling of the SuperI/O functions.
Note that all SuperI/O functions are now deactivated upon reset (except for
VT82C686B's serial ports where Fuloong 2e's rescue-yl seems to expect them to be
enabled by default). Rely on firmware -- or in case of pegasos2 on board code if
no -bios is given -- to configure the functions accordingly.
Pegasos2 emulates firmware when no -bios is given, this was explained in
previos commit so maybe not needed to be explained it here again so you could
drop the comment between -- -- but I don't mind.
Signed-off-by: Bernhard Beschow
---
hw/isa/vt82c686.c | 121 ++
1 file changed, 90 insertions(+), 31 deletions(-)
diff --git a/hw/isa/vt82c686.c b/hw/isa/vt82c686.c
index 9c2333a277..be202d23cf 100644
--- a/hw/isa/vt82c686.c
+++ b/hw/isa/vt82c686.c
@@ -15,6 +15,9 @@
#include "qemu/osdep.h"
#include "hw/isa/vt82c686.h"
+#include "hw/block/fdc.h"
+#include "hw/char/parallel-isa.h"
+#include "hw/char/serial.h"
#include "hw/pci/pci.h"
#include "hw/qdev-properties.h"
#include "hw/ide/pci.h"
@@ -343,6 +346,35 @@ static const TypeInfo via_superio_info = {
#define TYPE_VT82C686B_SUPERIO "vt82c686b-superio"
+static void vt82c686b_superio_update(ViaSuperIOState *s)
+{
+isa_parallel_set_enabled(s->superio.parallel[0],
+ (s->regs[0xe2] & 0x3) != 3);
+isa_serial_set_enabled(s->superio.serial[0], s->regs[0xe2] & BIT(2));
+isa_serial_set_enabled(s->superio.serial[1], s->regs[0xe2] & BIT(3));
+isa_fdc_set_enabled(s->superio.floppy, s->regs[0xe2] & BIT(4));
+
+isa_fdc_set_iobase(s->superio.floppy, (s->regs[0xe3] & 0xfc) << 2);
+isa_parallel_set_iobase(s->superio.parallel[0], s->regs[0xe6] << 2);
+isa_serial_set_iobase(s->superio.serial[0], (s->regs[0xe7] & 0xfe) << 2);
+isa_serial_set_iobase(s->superio.serial[1], (s->regs[0xe8] & 0xfe) << 2);
+}
I wonder if some code duplication could be saved by adding a shared
via_superio_update() for this further up in the abstract via-superio class
instead of this method and vt8231_superio_update() below. This common method in
abstract class would need to handle the differences which seem to be reg
addresses offset by 0x10 and VT8231 having only 1 serial port. These could
either be handled by adding function parameters or fields to ViaSuperIOState
for this that the subclasses can set and the method check. (Such as reg
base=0xe2 for vt82c686 and 0xf2 for vt8231 and num_serial or similar for how
many ports are there then can have a for loop for those that would only run
once for vt8231).
Only the enable bits and the parallel port base address line up, the serial
port(s) and the floppy would need special treatment. Not worth it IMO.
Missed this part in previous reply. The serial ports would be taken care of by
a loop for number of ports so only the floppy needs an if which could also use
the number of serial ports for lack of better way to distinguish these cips
easily. Number of ports are in the superio class which you could get with
ISA_SUPERIO_GET_CLASS (see via_superio_realize) then serial.count would be 2
for 686b and 1 for 8231.
I'm not very convinced about telling the device models apart by their number of
sub devices. So let's omit this part for now.
But now I think another way may be better that is to drop the
superio_update function as this updates all functions on writing any
register unnecessarily and put the lines from it in the
vt*_superio_cfg_write() functions under the separate cases. This was
the original intent, that's why the reset function goes through that
write function so it can enable/disable functions. That way you could
apply mask on write so via_superio_cfg_read() would return 0 bits as 0
(although the data sheet is not clear about what real chip does, just
says these must be 0 not that it's enforced but if we enforce that it's
probably better to return the effective value on read as well). Then
when state saving is added in separate patch you can have a similar
function as vt82c686b_superio_reset() (or rename that to update and
make it use regs[xx] instead of constant values and call that from
reset after setting regs values like you did here. But that needs more
thought as the vmstate added
Re: [RFC 0/3] aio-posix: call ->poll_end() when removing AioHandler
Am 02.01.24 um 16:24 schrieb Hanna Czenczek:
>
> I’ve attached the preliminary patch that I didn’t get to send (or test
> much) last year. Not sure if it has the same CPU-usage-spike issue
> Fiona was seeing, the only functional difference is that I notify the vq
> after attaching the notifiers instead of before.
>
Applied the patch on top of c12887e1b0 ("block-coroutine-wrapper: use
qemu_get_current_aio_context()") because it conflicts with b6948ab01d
("virtio-blk: add iothread-vq-mapping parameter").
I'm happy to report that I cannot reproduce the CPU-usage-spike issue
with the patch, but I did run into an assertion failure when trying to
verify that it fixes my original stuck-guest-IO issue. See below for the
backtrace [0]. Hanna wrote in https://issues.redhat.com/browse/RHEL-3934
> I think it’s sufficient to simply call virtio_queue_notify_vq(vq) after the
> virtio_queue_aio_attach_host_notifier(vq, ctx) call, because both
> virtio-scsi’s and virtio-blk’s .handle_output() implementations acquire the
> device’s context, so this should be directly callable from any context.
I guess this is not true anymore now that the AioContext locking was
removed?
Back to the CPU-usage-spike issue: I experimented around and it doesn't
seem to matter whether I notify the virt queue before or after attaching
the notifiers. But there's another functional difference. My patch
called virtio_queue_notify() which contains this block:
> if (vq->host_notifier_enabled) {
> event_notifier_set(>host_notifier);
> } else if (vq->handle_output) {
> vq->handle_output(vdev, vq);
In my testing, the first branch was taken, calling event_notifier_set().
Hanna's patch uses virtio_queue_notify_vq() and there,
vq->handle_output() will be called. That seems to be the relevant
difference regarding the CPU-usage-spike issue.
Best Regards,
Fiona
[0]:
> #0 __pthread_kill_implementation (threadid=,
> signo=signo@entry=6, no_tid=no_tid@entry=0) at ./nptl/pthread_kill.c:44
> #1 0x760e3d9f in __pthread_kill_internal (signo=6,
> threadid=) at ./nptl/pthread_kill.c:78
> #2 0x76094f32 in __GI_raise (sig=sig@entry=6) at
> ../sysdeps/posix/raise.c:26
> #3 0x7607f472 in __GI_abort () at ./stdlib/abort.c:79
> #4 0x7607f395 in __assert_fail_base (fmt=0x761f3a90 "%s%s%s:%u:
> %s%sAssertion `%s' failed.\n%n",
> assertion=assertion@entry=0x56246bf8 "ctx ==
> qemu_get_current_aio_context()",
> file=file@entry=0x56246baf "../system/dma-helpers.c",
> line=line@entry=123,
> function=function@entry=0x56246c70 <__PRETTY_FUNCTION__.1>
> "dma_blk_cb") at ./assert/assert.c:92
> #5 0x7608de32 in __GI___assert_fail (assertion=0x56246bf8 "ctx
> == qemu_get_current_aio_context()",
> file=0x56246baf "../system/dma-helpers.c", line=123,
> function=0x56246c70 <__PRETTY_FUNCTION__.1> "dma_blk_cb")
> at ./assert/assert.c:101
> #6 0x55b83425 in dma_blk_cb (opaque=0x5804f150, ret=0) at
> ../system/dma-helpers.c:123
> #7 0x55b839ec in dma_blk_io (ctx=0x57404310, sg=0x588ca6f8,
> offset=70905856, align=512,
> io_func=0x55a94a87 , io_func_opaque=0x5817ea00,
> cb=0x55a8d99f , opaque=0x5817ea00,
> dir=DMA_DIRECTION_FROM_DEVICE) at ../system/dma-helpers.c:236
> #8 0x55a8de9a in scsi_do_read (r=0x5817ea00, ret=0) at
> ../hw/scsi/scsi-disk.c:431
> #9 0x55a8e249 in scsi_read_data (req=0x5817ea00) at
> ../hw/scsi/scsi-disk.c:501
> #10 0x55a897e3 in scsi_req_continue (req=0x5817ea00) at
> ../hw/scsi/scsi-bus.c:1478
> #11 0x55d8270e in virtio_scsi_handle_cmd_req_submit
> (s=0x58669af0, req=0x588ca6b0) at ../hw/scsi/virtio-scsi.c:828
> #12 0x55d82937 in virtio_scsi_handle_cmd_vq (s=0x58669af0,
> vq=0x58672550) at ../hw/scsi/virtio-scsi.c:870
> #13 0x55d829a9 in virtio_scsi_handle_cmd (vdev=0x58669af0,
> vq=0x58672550) at ../hw/scsi/virtio-scsi.c:883
> #14 0x55db3784 in virtio_queue_notify_vq (vq=0x58672550) at
> ../hw/virtio/virtio.c:2268
> #15 0x55d8346a in virtio_scsi_drained_end (bus=0x58669d88) at
> ../hw/scsi/virtio-scsi.c:1179
> #16 0x55a8a549 in scsi_device_drained_end (sdev=0x58105000) at
> ../hw/scsi/scsi-bus.c:1774
> #17 0x55a931db in scsi_disk_drained_end (opaque=0x58105000) at
> ../hw/scsi/scsi-disk.c:2369
> #18 0x55ee439c in blk_root_drained_end (child=0x574065d0) at
> ../block/block-backend.c:2829
> #19 0x55ef0ac3 in bdrv_parent_drained_end_single (c=0x574065d0)
> at ../block/io.c:74
> #20 0x55ef0b02 in bdrv_parent_drained_end (bs=0x57409f80,
> ignore=0x0) at ../block/io.c:89
> #21 0x55ef1b1b in bdrv_do_drained_end (bs=0x57409f80, parent=0x0)
> at ../block/io.c:421
> #22 0x55ef1b5a in bdrv_drained_end (bs=0x57409f80) at
> ../block/io.c:428
> #23 0x55efcf64 in
Re: [PATCH v3 0/2] block: commit/stream: Allow users to request only format driver names in backing file format
On Tue, Dec 05, 2023 at 18:14:40 +0100, Peter Krempa wrote: > Please see patches for rationale. > > Libvirt patches using this new flag will be posted soon-ish (after > cleanup). > > v3: > - changed name of flag to 'backing-mask-protocol' (Eric) > - decided to keep Vladimir's R-b as he requested shorter name too > > v2: > - fixed mistaken argument order in 'hmp_block_stream' > - changed version in docs to 9.0 as getting this into RC 3 probably >isn't realistic > > Peter Krempa (2): > block: commit: Allow users to request only format driver names in > backing file format > block: stream: Allow users to request only format driver names in > backing file format Polite ping, now that qemu-8.2 was released.
