Re: [PATCH v3 07/11] hw/sh4/r2d: Realize IDE controller before accessing it
Hi, On Thu, Feb 08, 2024 at 07:12:40PM +0100, Philippe Mathieu-Daudé wrote: > We should not wire IRQs on unrealized device. > > Signed-off-by: Philippe Mathieu-Daudé > Reviewed-by: Peter Maydell > Reviewed-by: Yoshinori Sato qemu 9.0 fails to boot Linux from ide/ata drives with the sh4 and sh4eb emulations. Error log is as follows. ata1.00: ATA-7: QEMU HARDDISK, 2.5+, max UDMA/100 ata1.00: 16384 sectors, multi 16: LBA48 ata1.00: configured for PIO scsi 0:0:0:0: Direct-Access ATA QEMU HARDDISK2.5+ PQ: 0 ANSI: 5 sd 0:0:0:0: [sda] 16384 512-byte logical blocks: (8.39 MB/8.00 MiB) sd 0:0:0:0: [sda] Write Protect is off sd 0:0:0:0: [sda] Write cache: enabled, read cache: enabled, doesn't support DPO or FUA ata1: lost interrupt (Status 0x58) [ and more similar errors ] qemu command line: qemu-system-sh4eb -M r2d -kernel arch/sh/boot/zImage \ -snapshot -drive file=rootfs.ext2,format=raw,if=ide \ -append "root=/dev/sda console=ttySC1,115200 noiotrap" \ -serial null -serial stdio -monitor null -nographic -no-reboot Bisect points to this patch (see below). Reverting it fixes the problem. Guenter --- bisect log: # bad: [c25df57ae8f9fe1c72eee2dab37d76d904ac382e] Update version for 9.0.0 release # good: [1600b9f46b1bd08b00fe86c46ef6dbb48cbe10d6] Update version for v8.2.0 release git bisect start 'v9.0.0' 'v8.2.0' # good: [62357c047a5abc6ede992159ed7c0aaaeb50617a] Merge tag 'qemu-sparc-20240213' of https://github.com/mcayland/qemu into staging git bisect good 62357c047a5abc6ede992159ed7c0aaaeb50617a # bad: [d65f1ed7de1559534d0a1fabca5bdd81c594c7ca] docs/acpi/bits: add some clarity and details while also improving formating git bisect bad d65f1ed7de1559534d0a1fabca5bdd81c594c7ca # bad: [99e1c1137b6f339be1e4b76e243ad7b7c3d3cb8c] hw/i386/pc: Populate RTC attribute directly git bisect bad 99e1c1137b6f339be1e4b76e243ad7b7c3d3cb8c # bad: [760b4dcdddba4a40b9fa0eb78fdfc7eda7cb83d0] Merge tag 'for-upstream' of https://gitlab.com/bonzini/qemu into staging git bisect bad 760b4dcdddba4a40b9fa0eb78fdfc7eda7cb83d0 # good: [f2b4a98930c122648e9dc494e49cea5dffbcc2be] target/arm: Allow access to SPSR_hyp from hyp mode git bisect good f2b4a98930c122648e9dc494e49cea5dffbcc2be # bad: [1a8e2f58c5dd721086284f827326b370d19ad9eb] hw/i386/q35: Use DEVICE() cast macro with PCIDevice object git bisect bad 1a8e2f58c5dd721086284f827326b370d19ad9eb # good: [59ae6bcddc3651b55b96c2bf05a6cd4312e46d10] hw/ppc/prep: Realize ISA bridge before accessing it git bisect good 59ae6bcddc3651b55b96c2bf05a6cd4312e46d10 # bad: [7ed9a5f626a6c932a8c869a91e6a8b3e2029f5ef] hw/intc/grlib_irqmp: implements the multiprocessor status register git bisect bad 7ed9a5f626a6c932a8c869a91e6a8b3e2029f5ef # bad: [d08b7af3f7f27f6f3da8446756bf0b9352026b1d] target/sparc: Provide hint about CPUSPARCState::irq_manager member git bisect bad d08b7af3f7f27f6f3da8446756bf0b9352026b1d # bad: [5e37bc4997c32a1c9a6621a060462c84df9f1b8f] hw/dma: Pass parent object to i8257_dma_init() git bisect bad 5e37bc4997c32a1c9a6621a060462c84df9f1b8f # bad: [3c5f86a22686ef475a8259c0d8ee714f61c770c9] hw/sh4/r2d: Realize IDE controller before accessing it git bisect bad 3c5f86a22686ef475a8259c0d8ee714f61c770c9 # good: [fc432ba0f58343c8912b80e9056315bb9bd8df92] hw/misc/macio: Realize IDE controller before accessing it git bisect good fc432ba0f58343c8912b80e9056315bb9bd8df92 # first bad commit: [3c5f86a22686ef475a8259c0d8ee714f61c770c9] hw/sh4/r2d: Realize IDE controller before accessing it
Re: [PATCH v2 5/5] monitor: use aio_co_reschedule_self()
Am 06.02.2024 um 20:06 hat Stefan Hajnoczi geschrieben: > The aio_co_reschedule_self() API is designed to avoid the race > condition between scheduling the coroutine in another AioContext and > yielding. > > The QMP dispatch code uses the open-coded version that appears > susceptible to the race condition at first glance: > > aio_co_schedule(qemu_get_aio_context(), qemu_coroutine_self()); > qemu_coroutine_yield(); > > The code is actually safe because the iohandler and qemu_aio_context > AioContext run under the Big QEMU Lock. Nevertheless, set a good example > and use aio_co_reschedule_self() so it's obvious that there is no race. > > Suggested-by: Hanna Reitz > Reviewed-by: Manos Pitsidianakis > Reviewed-by: Hanna Czenczek > Signed-off-by: Stefan Hajnoczi > --- > qapi/qmp-dispatch.c | 7 ++- > 1 file changed, 2 insertions(+), 5 deletions(-) > > diff --git a/qapi/qmp-dispatch.c b/qapi/qmp-dispatch.c > index 176b549473..f3488afeef 100644 > --- a/qapi/qmp-dispatch.c > +++ b/qapi/qmp-dispatch.c > @@ -212,8 +212,7 @@ QDict *coroutine_mixed_fn qmp_dispatch(const > QmpCommandList *cmds, QObject *requ > * executing the command handler so that it can make progress if > it > * involves an AIO_WAIT_WHILE(). > */ > -aio_co_schedule(qemu_get_aio_context(), qemu_coroutine_self()); > -qemu_coroutine_yield(); > +aio_co_reschedule_self(qemu_get_aio_context()); Turns out that this one actually causes a regression. [1] This code is ŕun in iohandler_ctx, aio_co_reschedule_self() looks at the new context and compares it with qemu_get_current_aio_context() - and because both are qemu_aio_context, it decides that it has nothing to do. So the command handler coroutine actually still runs in iohandler_ctx now, which is not what we want. We could just revert this patch because it was only meant as a cleanup without a semantic difference. Or aio_co_reschedule_self() could look at qemu_coroutine_self()->ctx instead of using qemu_get_current_aio_context(). That would be a little more indirect, though, and I'm not sure if co->ctx is always up to date. Any opinions on what is the best way to fix this? Kevin [1] https://issues.redhat.com/browse/RHEL-34618
Re: [PATCH-for-9.1 v2 2/3] migration: Remove RDMA protocol handling
On Fri, May 03, 2024 at 08:40:03AM +0200, Jinpu Wang wrote: > I had a brief check in the rsocket changelog, there seems some > improvement over time, > might be worth revisiting this. due to socket abstraction, we can't > use some feature like > ODP, it won't be a small and easy task. It'll be good to know whether Dan's suggestion would work first, without rewritting everything yet so far. Not sure whether some perf test could help with the rsocket APIs even without QEMU's involvements (or looking for test data supporting / invalidate such conversions). Thanks, -- Peter Xu
Re: [PULL v2 03/16] block/block-backend: add block layer APIs resembling Linux ZonedBlockDevice ioctls
On Mon, 15 May 2023 at 17:07, Stefan Hajnoczi wrote:
>
> From: Sam Li
>
> Add zoned device option to host_device BlockDriver. It will be presented only
> for zoned host block devices. By adding zone management operations to the
> host_block_device BlockDriver, users can use the new block layer APIs
> including Report Zone and four zone management operations
> (open, close, finish, reset, reset_all).
>
> Qemu-io uses the new APIs to perform zoned storage commands of the device:
> zone_report(zrp), zone_open(zo), zone_close(zc), zone_reset(zrs),
> zone_finish(zf).
>
> For example, to test zone_report, use following command:
> $ ./build/qemu-io --image-opts -n driver=host_device, filename=/dev/nullb0
> -c "zrp offset nr_zones"
Hi; Coverity points out an issue in this commit (CID 1544771):
> +static int zone_report_f(BlockBackend *blk, int argc, char **argv)
> +{
> +int ret;
> +int64_t offset;
> +unsigned int nr_zones;
> +
> +++optind;
> +offset = cvtnum(argv[optind]);
> +++optind;
> +nr_zones = cvtnum(argv[optind]);
cvtnum() can fail and return a negative value on error
(e.g. if the number in the string is out of range),
but we are not checking for that. Instead we stuff
the value into an 'unsigned int' and then pass that to
g_new(), which will result in our trying to allocate a large
amount of memory.
Here, and also in the other functions below that use cvtnum(),
I think we should follow the pattern for use of that function
that is used in the pre-existing code in this function:
int64_t foo; /* NB: not an unsigned or some smaller type */
foo = cvtnum(arg)
if (foo < 0) {
print_cvtnum_err(foo, arg);
return foo; /* or otherwise handle returning an error upward */
}
It looks like all the uses of cvtnum in this patch should be
adjusted to handle errors.
thanks
-- PMM
Re: [PATCH-for-9.1 v2 2/3] migration: Remove RDMA protocol handling
Hi Daniel, On Wed, May 1, 2024 at 6:00 PM Daniel P. Berrangé wrote: > > On Wed, May 01, 2024 at 11:31:13AM -0400, Peter Xu wrote: > > What I worry more is whether this is really what we want to keep rdma in > > qemu, and that's also why I was trying to request for some serious > > performance measurements comparing rdma v.s. nics. And here when I said > > "we" I mean both QEMU community and any company that will support keeping > > rdma around. > > > > The problem is if NICs now are fast enough to perform at least equally > > against rdma, and if it has a lower cost of overall maintenance, does it > > mean that rdma migration will only be used by whoever wants to keep them in > > the products and existed already? In that case we should simply ask new > > users to stick with tcp, and rdma users should only drop but not increase. > > > > It seems also destined that most new migration features will not support > > rdma: see how much we drop old features in migration now (which rdma > > _might_ still leverage, but maybe not), and how much we add mostly multifd > > relevant which will probably not apply to rdma at all. So in general what > > I am worrying is a both-loss condition, if the company might be easier to > > either stick with an old qemu (depending on whether other new features are > > requested to be used besides RDMA alone), or do periodic rebase with RDMA > > downstream only. > > I don't know much about the originals of RDMA support in QEMU and why > this particular design was taken. It is indeed a huge maint burden to > have a completely different code flow for RDMA with 4000+ lines of > custom protocol signalling which is barely understandable. > > I would note that /usr/include/rdma/rsocket.h provides a higher level > API that is a 1-1 match of the normal kernel 'sockets' API. If we had > leveraged that, then QIOChannelSocket class and the QAPI SocketAddress > type could almost[1] trivially have supported RDMA. There would have > been almost no RDMA code required in the migration subsystem, and all > the modern features like compression, multifd, post-copy, etc would > "just work". I guess at the time rsocket is less mature, and less performant compared to using uverbs directly. > > I guess the 'rsocket.h' shim may well limit some of the possible > performance gains, but it might still have been a better tradeoff > to have not quite so good peak performance, but with massively > less maint burden. I had a brief check in the rsocket changelog, there seems some improvement over time, might be worth revisiting this. due to socket abstraction, we can't use some feature like ODP, it won't be a small and easy task. > With regards, > Daniel Thanks for the suggestion. > > [1] "almost" trivially, because the poll() integration for rsockets > requires a bit more magic sauce since rsockets FDs are not > really FDs from the kernel's POV. Still, QIOCHannel likely can > abstract that probme. > -- > |: 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 :| >
