versatile: Push lsi initialization to the end
PATCH 1/1 versatile: Push lsi initialization to the end
This is nasty, but there is no better way given current mux logic:
As setting up the block device will trigger a qemu_bh_poll while there are qemu_chr open events in the queue, we have to register the UARTs and everything else that might be mux'ed first so that the right active frontend is already registered when the bottom-half is finally processed.
This fixes spurious monitor messages with
qemu-system-arm ... -machine versatilepb /path/to/disk -nographic
--- hw/versatilepb.c | 11 ++++++----- 1 files changed, 6 insertions(+), 5 deletions(-)
hw/versatilepb.c +6 -5
@@ -250,11 +250,6 @@ static void versatile_init(ram_addr_t ram_size,if (usb_enabled) {pci_create_simple (pci_bus,-1 ,"pci-ohci" ); } - n =drive_get_max_bus (IF_SCSI ); -while (n >=0 ) { -pci_create_simple (pci_bus,-1 ,"lsi53c895a" ); - n--; - }sysbus_create_simple ("pl011" ,0x101f1000 , pic[12 ]);sysbus_create_simple ("pl011" ,0x101f2000 , pic[13 ]);
@@ -325,6 +320,12 @@ static void versatile_init(ram_addr_t ram_size,/* 0x101f4000 SSPI. */ /* 0x34000000 NOR Flash */ + n =drive_get_max_bus (IF_SCSI ); +while (n >=0 ) { +pci_create_simple (pci_bus,-1 ,"lsi53c895a" ); + n--; + } + dinfo =drive_get (IF_PFLASH ,0 ,0 ); if (!pflash_cfi01_register(VERSATILE_FLASH_ADDR, NULL, "versatile.flash", VERSATILE_FLASH_SIZE, dinfo ? dinfo->bdrv : NULL,
Signature · 1 line
1.7.3.4
Quoted 7 lines · Jan Kiszka
This is nasty, but there is no better way given current mux logic:
As setting up the block device will trigger a
qemu_bh_pollwhile there areqemu_chropen events in the queue, we have to register the UARTs and everything else that might be mux'ed first so that the right active frontend is already registered when the bottom-half is finally processed.
Yuck. I really don't like this -- why should the board model have to care about implementation internals of lsi53c895a? It's just plugging in a PCI card. And why only this device and not every storage device in every board?
Nothing should actually start happening until the whole machine model is completely set up and we've returned control to vl.c (and probably not until we've done our first system reset). Any device which is doing stuff early is broken and needs fixing.
-- PMM
Quoted 14 lines · Peter Maydell
Quoted 7 lines · Jan Kiszka
This is nasty, but there is no better way given current mux logic:
As setting up the block device will trigger a
qemu_bh_pollwhile there areqemu_chropen events in the queue, we have to register the UARTs and everything else that might be mux'ed first so that the right active frontend is already registered when the bottom-half is finally processed.Yuck. I really don't like this -- why should the board model have to care about implementation internals of lsi53c895a? It's just plugging in a PCI card. And why only this device and not every storage device in every board?
Quoted 4 lines
Nothing should actually start happening until the whole machine model is completely set up and we've returned control to
vl.c(and probably not until we've done our first system reset). Any device which is doing stuff early is broken and needs fixing.
Here is the backtrace that bites:
(gdb) bt
#0 qemu_bh_poll () at /data/qemu/async.c:56 #1 0x00005555555de819 in qemu_aio_wait () at /data/qemu/aio.c:118 #2 0x00005555555f6635 in bdrv_rw_co (bs=0x5555564cc140, sector_num=<optimized out>, buf=<optimized out>, nb_sectors=<optimized out>, is_write=<optimized out>) at /data/qemu/block.c:1819 #3 0x00005555555f6b1b in bdrv_read_unthrottled (bs=0x5555564cc140, sector_num=<optimized out>, buf=<optimized out>, nb_sectors=<optimized out>) at /data/qemu/block.c:1841 #4 0x0000555555637a8a in guess_disk_lchs (bs=0x5555564cc140, pcylinders=0x7fffffffd434, pheads=0x7fffffffd430, psectors=0x7fffffffd42c) at /data/qemu/hw/hd-geometry.c:68 #5 0x0000555555637c10 in hd_geometry_guess (bs=0x5555564cc140, pcyls=0x5555565dbbac, pheads=0x5555565dbbb0, psecs=0x5555565dbbb4, ptrans=0x0) at /data/qemu/hw/hd-geometry.c:125 #6 0x000055555562d0da in blkconf_geometry (conf=0x5555565dbb90, ptrans=0x0, cyls_max=65535, heads_max=255, secs_max=255) at /data/qemu/hw/block-common.c:43 #7 0x0000555555648dea in scsi_initfn (dev=0x5555565dbaf0) at /data/qemu/hw/scsi-disk.c:2061 #8 0x00005555556452e6 in scsi_device_init (s=0x5555565dbaf0) at /data/qemu/hw/scsi-bus.c:42 #9 scsi_qdev_init (qdev=<optimized out>) at /data/qemu/hw/scsi-bus.c:183
#10 0x0000555555641e7f in qdev_init (dev=0x5555565dbaf0) at /data/qemu/hw/qdev.c:160
#11 0x00005555556428c3 in scsi_bus_legacy_add_drive (bus=<optimized out>, bdrv=0x5555564cc140, unit=0, removable=false, bootindex=-1) at /data/qemu/hw/scsi-bus.c:228 #12 0x00005555556429d8 in scsi_bus_legacy_handle_cmdline (bus=0x5555565d92b8) at /data/qemu/hw/scsi-bus.c:246
#13 0x00005555556c1a46 in pci_qdev_init (qdev=0x5555565d8b00) at /data/qemu/hw/pci.c:1556
#14 0x0000555555641e7f in qdev_init (dev=0x5555565d8b00) at /data/qemu/hw/qdev.c:160
#15 0x0000555555641fd1 in qdev_init_nofail (dev=0x5555565d8b00) at /data/qemu/hw/qdev.c:261
#16 0x00005555556c25b8 in pci_create_simple_multifunction (bus=<optimized out>, devfn=<optimized out>, multifunction=<optimized out>, name=<optimized out>) at /data/qemu/hw/pci.c:1615 #17 0x0000555555808f44 in versatile_init (ram_size=134217728, boot_device=<optimized out>,
kernel_filename=0x5555564c9420 "zImage-qemuarm.bin", kernel_cmdline=0x5555558a0558 "", initrd_filename=0x0,
cpu_model=<optimized out>, board_id=387) at /data/qemu/hw/arm/../versatilepb.c:325
#18 0x0000555555809100 in vpb_init (ram_size=<optimized out>, boot_device=<optimized out>, kernel_filename=<optimized out>, kernel_cmdline=<optimized out>, initrd_filename=<optimized out>,
cpu_model=<optimized out>) at /data/qemu/hw/arm/../versatilepb.c:351#19 0x000055555570bcc9 in main (argc=<optimized out>, argv=<optimized out>, envp=<optimized out>) at /data/qemu/vl.c:3627
There are earlier calls to qemu_bh_poll, but likely before chardev creation.
I'm not a fan of this either, but the alternatives are way more complicated. We either need to rewrite the chardev subsystem, specifically how mux'ed devices are registered and how the active one is selected. Or we need to avoid flushing "unrelated" BHs for block devices. Not sure of those read requests can be postponed.
Jan
Signature · 2 lines
Siemens AG, Corporate Technology, CT RTC ITP SDP-DE Corporate Competence Center Embedded Linux
Quoted 5 lines · Jan Kiszka
I'm not a fan of this either, but the alternatives are way more complicated. We either need to rewrite the chardev subsystem, specifically how mux'ed devices are registered and how the active one is selected. Or we need to avoid flushing "unrelated" BHs for block devices. Not sure of those read requests can be postponed.
Is this a regression? If it is then the obvious answer is to back out whatever broke it...
-- PMM
Quoted 9 lines · Peter Maydell
Quoted 5 lines · Jan Kiszka
I'm not a fan of this either, but the alternatives are way more complicated. We either need to rewrite the chardev subsystem, specifically how mux'ed devices are registered and how the active one is selected. Or we need to avoid flushing "unrelated" BHs for block devices. Not sure of those read requests can be postponed.
Is this a regression? If it is then the obvious answer is to back out whatever broke it...
I'm using this machine for the first time, so I cannot answer this from the top of my head. However, I don't think it can be a regression.
Mux chardevs work like this: You create the backend, then you register the frontend with them, one by one. The last one registered is the first one active. It should also receive the open event of chardev. But as that open even is issued via a BH and last frontend, the serial device, arrives after the first BH flushing, things break.
Jan
Signature · 2 lines
Siemens AG, Corporate Technology, CT RTC ITP SDP-DE Corporate Competence Center Embedded Linux
Il 08/10/2012 08:52, Jan Kiszka ha scritto:
Quoted 11 lines · Peter Maydell
Quoted 9 lines · Jan Kiszka
Quoted 5 lines
I'm not a fan of this either, but the alternatives are way more complicated. We either need to rewrite the chardev subsystem, specifically how mux'ed devices are registered and how the active one is selected. Or we need to avoid flushing "unrelated" BHs for block devices. Not sure of those read requests can be postponed.
Is this a regression? If it is then the obvious answer is to back out whatever broke it...
Quoted 8 lines
I'm using this machine for the first time, so I cannot answer this from the top of my head. However, I don't think it can be a regression.
Mux chardevs work like this: You create the backend, then you register the frontend with them, one by one. The last one registered is the first one active. It should also receive the open event of chardev. But as that open even is issued via a BH and last frontend, the serial device, arrives after the first BH flushing, things break.
Does something like this work instead?
qemu-char.c +0 -0
@@ -465,6 +465,9 @@ static void
mux_chr_update_read_handler(CharDriverState *chr)
d->focus = d->mux_cnt;
d->mux_cnt++;
mux_chr_send_event(d, d->focus, CHR_EVENT_MUX_IN);+ if (chr->opened) {
+ mux_chr_send_event(d, d->focus, CHR_EVENT_OPENED);
+ }
}
static CharDriverState *qemu_chr_open_mux(CharDriverState *drv)
Paolo
Quoted 28 lines · Paolo Bonzini
Il 08/10/2012 08:52, Jan Kiszka ha scritto:
Quoted 11 lines · Peter Maydell
Quoted 9 lines · Jan Kiszka
Quoted 5 lines
I'm not a fan of this either, but the alternatives are way more complicated. We either need to rewrite the chardev subsystem, specifically how mux'ed devices are registered and how the active one is selected. Or we need to avoid flushing "unrelated" BHs for block devices. Not sure of those read requests can be postponed.
Is this a regression? If it is then the obvious answer is to back out whatever broke it...
Quoted 8 lines
I'm using this machine for the first time, so I cannot answer this from the top of my head. However, I don't think it can be a regression.
Mux chardevs work like this: You create the backend, then you register the frontend with them, one by one. The last one registered is the first one active. It should also receive the open event of chardev. But as that open even is issued via a BH and last frontend, the serial device, arrives after the first BH flushing, things break.
Does something like this work instead?
qemu-char.c +0
-0
qemu-char.c static void · line 471
Quoted 7 lines
qemu-char.c +3
-0@@ -465,6 +465,9 @@ static voidmux_chr_update_read_handler (CharDriverState *chr) d->focus = d->mux_cnt; d->mux_cnt++;mux_chr_send_event (d, d->focus,CHR_EVENT_MUX_IN ); +if (chr->opened) { +mux_chr_send_event (d, d->focus,CHR_EVENT_OPENED ); + }
It's not (only) about a missing event for the serial frontend, it's also about a spurious open event to the monitor. That generates unwanted output during startup.
Jan
Signature · 2 lines
Siemens AG, Corporate Technology, CT RTC ITP SDP-DE Corporate Competence Center Embedded Linux
Quoted 3 lines · Jan Kiszka
It's not (only) about a missing event for the serial frontend, it's also about a spurious open event to the monitor. That generates unwanted output during startup.
Right, so whatever's sending that event needs not to do so until creation of the machine is complete. Sending events randomly halfway through init is a recipe for problems...
-- PMM
Il 08/10/2012 18:02, Peter Maydell ha scritto:
Quoted 6 lines
Quoted 3 lines
Quoted 3 lines
It's not (only) about a missing event for the serial frontend, it's also about a spurious open event to the monitor. That generates unwanted output during startup.
Right, so whatever's sending that event needs not to do so until creation of the machine is complete. Sending events randomly halfway through init is a recipe for problems...
So we now have proof that using bottom halves outside the block layer was a bad idea. Thanks Jan. :)
If Stefan goes on with the AioContext idea, we can have separate AioContexts for qemu_bh_new on one side and for block devices on the other, so that qemu_bh_new bottom halves won't fire just because something is calling the block layer.
In the meanwhile, qemu_char could use a QEMUTimer expiring in the past instead of a bottom half.
Paolo
Quoted 11 lines · Paolo Bonzini
Il 08/10/2012 18:02, Peter Maydell ha scritto:
Quoted 6 lines
Quoted 3 lines
Quoted 3 lines
It's not (only) about a missing event for the serial frontend, it's also about a spurious open event to the monitor. That generates unwanted output during startup.
Right, so whatever's sending that event needs not to do so until creation of the machine is complete. Sending events randomly halfway through init is a recipe for problems...
So we now have proof that using bottom halves outside the block layer was a bad idea. Thanks Jan. :)
Quoted 7 lines
If Stefan goes on with the AioContext idea, we can have separate AioContexts for
qemu_bh_newon one side and for block devices on the other, so thatqemu_bh_newbottom halves won't fire just because something is calling the block layer.In the meanwhile,
qemu_charcould use a QEMUTimer expiring in the past instead of a bottom half.
Sounds "beautiful", waiting to break when we leave like this and start playing with the timer subsystem ;). But I guess it's best for now. Let me have a look...
Jan
Signature · 2 lines
Siemens AG, Corporate Technology, CT RTC ITP SDP-DE Corporate Competence Center Embedded Linux
Quoted 8 lines · Jan Kiszka
Quoted 7 lines · Peter Maydell
Quoted 5 lines · Jan Kiszka
I'm not a fan of this either, but the alternatives are way more complicated. We either need to rewrite the chardev subsystem, specifically how mux'ed devices are registered and how the active one is selected. Or we need to avoid flushing "unrelated" BHs for block devices. Not sure of those read requests can be postponed.
Quoted 6 lines
Quoted 2 lines
Is this a regression? If it is then the obvious answer is to back out whatever broke it...
I'm using this machine for the first time, so I cannot answer this from the top of my head. However, I don't think it can be a regression.
What is the bug exactly? Outputting the monitor prompt when using -nographic? If it is the case, please note that mips/mipsel is also affected. IIRC it appears somewhere between 1.1 and 1.2.
Signature · 2 lines
Aurelien Jarno GPG: 1024D/F1BCDB73 [email protected] http://www.aurel32.net
Quoted 12 lines · Aurelien Jarno
Quoted 11 lines · Jan Kiszka
Quoted 9 lines · Peter Maydell
Quoted 5 lines · Jan Kiszka
I'm not a fan of this either, but the alternatives are way more complicated. We either need to rewrite the chardev subsystem, specifically how mux'ed devices are registered and how the active one is selected. Or we need to avoid flushing "unrelated" BHs for block devices. Not sure of those read requests can be postponed.
Is this a regression? If it is then the obvious answer is to back out whatever broke it...
Quoted 7 lines
Quoted 3 lines
I'm using this machine for the first time, so I cannot answer this from the top of my head. However, I don't think it can be a regression.
What is the bug exactly? Outputting the monitor prompt when using -nographic? If it is the case, please note that mips/mipsel is also affected. IIRC it appears somewhere between 1.1 and 1.2.
-nographic (-serial mon:stdio) + SCSI disk, at least that was the combination here.
See http://thread.gmane.org/gmane.comp.emulators.qemu/174687 for a more generic, well, workaround.
Jan
Signature · 2 lines
Siemens AG, Corporate Technology, CT RTC ITP SDP-DE Corporate Competence Center Embedded Linux
Quoted 7 lines · Jan Kiszka
This is nasty, but there is no better way given current mux logic:
As setting up the block device will trigger a
qemu_bh_pollwhile there areqemu_chropen events in the queue, we have to register the UARTs and everything else that might be mux'ed first so that the right active frontend is already registered when the bottom-half is finally processed.
So I guess this comes down to what the semantics of bottom halves are. I can see two plausible options:
1. bottom halves are a mechanism provided by our cpu/device simulation framework, and so will never be run before the simulation is fully initialised
* this means devices can register BHs which set irq lines, send events to chr mux front ends etc etc
* it also means that device setup mustn't trigger a bh_poll (so we'd need to track down the bit of the block device setup that's causing this)
2. bottom halves are a generic mechanism that you can use not just as part of the simulation, and so BHs may run as soon as they're registered
* this would let us use them for arbitrary purposes in init
* we'd need to audit and fix all the current uses to check whether they're safe to run early or if they need to have a 'do nothing if simulation not running' check
Any opinions?
-- PMM
Il 08/10/2012 18:33, Peter Maydell ha scritto:
Quoted 21 lines · Jan Kiszka
Quoted 7 lines
This is nasty, but there is no better way given current mux logic:
As setting up the block device will trigger a
qemu_bh_pollwhile there areqemu_chropen events in the queue, we have to register the UARTs and everything else that might be mux'ed first so that the right active frontend is already registered when the bottom-half is finally processed.So I guess this comes down to what the semantics of bottom halves are. I can see two plausible options:
1. bottom halves are a mechanism provided by our cpu/device simulation framework, and so will never be run before the simulation is fully initialised
* this means devices can register BHs which set irq lines, send events to chr mux front ends etc etc
* it also means that device setup mustn't trigger abh_poll(so we'd need to track down the bit of the block device setup that's causing this)
Quoted 7 lines
2. bottom halves are a generic mechanism that you can use not just as part of the simulation, and so BHs may run as soon as they're registered
* this would let us use them for arbitrary purposes in init
* we'd need to audit and fix all the current uses to check whether they're safe to run early or if they need to have a 'do nothing if simulation not running' check
3. bottom halves are an internal concept to the block layer that has been hijacked by device models. The bottom half idea is that the code would run as soon as the current code is done with this subsystem; ideally, you would instead queue a work item in a thread-pool and the code would block on the same fine-grained lock as the subsystem that created the bottom half. Work items from different subsystems would be able to run concurrently---of course that's not too helpful while we have a single lock for the whole iothread...
Stefan's work should be able to kill qemu_bh_new inside the block layer (replacing it with aio_bh_new), so qemu_bh_new can be repurposed to something that doesn't conflict with the block layer.
Paolo