Re: [RFC 0/3] aio-posix: call ->poll_end() when removing AioHandler

2024-01-03 Thread Paolo Bonzini

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

2024-01-03 Thread BALATON Zoltan

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 by

Re: [RFC 0/3] aio-posix: call ->poll_end() when removing AioHandler

2024-01-03 Thread Fiona Ebner
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(&vq->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

2024-01-03 Thread Peter Krempa
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.