From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:33411) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Z61tQ-0006UF-G5 for qemu-devel@nongnu.org; Fri, 19 Jun 2015 15:24:02 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Z61tO-00070h-Jy for qemu-devel@nongnu.org; Fri, 19 Jun 2015 15:24:00 -0400 Message-ID: <55846C47.3010801@redhat.com> Date: Fri, 19 Jun 2015 15:23:51 -0400 From: John Snow MIME-Version: 1.0 References: <55845CBF.8010906@redhat.com> <558463E1.5000507@redhat.com> <5584691F.4040801@redhat.com> In-Reply-To: <5584691F.4040801@redhat.com> Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable Subject: Re: [Qemu-devel] another isa-fdc problem... List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Laszlo Ersek , qemu devel list Cc: Kevin Wolf , =?UTF-8?B?SsOhbiBUb21rbw==?= , Markus Armbruster , qemu-block@nongnu.org, Eduardo Habkost On 06/19/2015 03:10 PM, Laszlo Ersek wrote: > On 06/19/15 20:48, John Snow wrote: >> >> >> On 06/19/2015 02:17 PM, Laszlo Ersek wrote: >>> With Eduardo's recent patch (473a49460db0a90bfda046b8f3662b49f94098eb= ), >>> q35 machtypes earlier than 2.4 work as expected. Also, pc-q35-2.4 wor= ks >>> fine in the default case (no board-default FDC, which is what we want= ), >>> and the traditional option "-drive if=3Dfloppy,..." also works as exp= ected. >>> >>> However, J=C3=A1n noticed that on pc-q35-2.4, the "canonical" >>> frontend/backend option pair >>> >>> -device isa-fdc,driveA=3Ddrive-fdc0-0-0 \ >>> -drive file=3D...,if=3Dnone,id=3Ddrive-fdc0-0-0,format=3Draw >>> >>> breaks guest OS detection of the FDC. Markus has now verified that th= e >>> qtree looks good (thanks for that), but the guest still doesn't notic= e >>> the FDC. >>> >>> In and >>> I wrote: >>> >>>> If you create an isa-fdc *outside* of the board init code, ie. outsi= de >>>> of the pc_q35_init() function, then in the following call chain: >>>> >>>> pc_q35_init() >>>> pc_basic_device_init() >>>> pc_cmos_init() >>>> ... >>>> rtc_set_memory(s, 0x10, val); >>>> >>>> the value stored at 0x10 in CMOS will not reflect the floppy. This i= s >>>> probably what trips up a guest OS -- they look to the CMOS for seein= g >>>> floppy presence. >>>> >>>> http://wiki.osdev.org/CMOS#Register_0x10 >>>> http://www.osdever.net/tutorials/view/detecting-floppy-drives >>>> >>>> In the guest kernel, "drivers/block/floppy.c" seems to be a heavy us= er >>>> of CMOS too. >>>> >>>> [...] >>>> >>>> BTW I did know (and document, in >>>> fd53c87cf6651b0dfe9f5107cfe77d2f697bd4f6) that pc_cmos_init() would >>>> get NULL for "floppy" (ie. FDC) if the board-default FDC was absent. >>>> What I didn't expect was that this would prevent a guest OS from >>>> seeing an FDC (without special module parameters) that was created >>>> differently. >>>> >>>> [...] >>> >>> Thus, I didn't regress earlier machine types, nor earlier command lin= es >>> with the new pc-q35-2.4 machine type, but I also didn't get right the >>> behavior for the "canonical" options that libvirt will want to use. >>> Markus suggested a way to fix that: >>> >>> off the cuff: the floppy code needs to move from pc_cmos_init() to >>> pc_cmos_init_late(), and find the isa-fdc regardless of how it was >>> added ... may have to walk qom data structures >>> >>> But before I try to do that, I looked at any preexistent test cases. >>> >>> Sure enough, tests/fdc-test.c catches this (in the "cmos" test), *if* >>> you hack it to start QEMU with -M q35. Therefore my first question is >>> what the best practice is for running a set of tests on different >>> machine types. >>> >>> QOSState / set_context() / qtest_pc_vboot() seem relevant (example: >>> "ahci-test.c"), but >>> - they also appear quite heavy weight, >> >> They're not /that/ heavy. They just set up an allocator and enable IRQ >> intercepts. If you don't use migrate or the allocator, the overhead >> should be pretty low. Just a lot of va_args shenanigans to make them >> easy to shoehorn into lots of scenarios. >> >>> - I'd lose (or have to open-code) the default options from qtest_init= (), >> >> qtest_pc_boot >> ..qtest_pc_vboot >> ....qtest_vboot >> ......qtest_start >> ........qtest_init >> >> You keep the default args when going through this chain, unless I am >> misunderstanding you. >=20 > You understood right; I didn't dig down this deep. Thanks. >=20 >>> - and I'd like to avoid: >>> - starting a separate qemu process for each single test, >>> - starting the necessary minimum number of qemu processes *in >>> parallel*. >>> >>> So some way would be necessary where I can add a bunch of test cases = for >>> different machine types, and gtester would stop the old qemu instance >>> and start the new qemu instance between tests when it notices that th= e >>> machine type changes from the previous test. I vaguely recall having >>> done something with GTester fixtures in the opts-visitor test, but it= 's >>> foggy. So, what's the best practice for this? Of course I could just >>> share data between tests, but that doesn't appear nice. >>> >> >> Are you sure you want to share QEMU instances between tests until we t= ry >> to change the boot options? I didn't really consider support for this >> inside of unit tests because I was encouraged to do full >> boot-up/shut-down so that each test would be independent. >=20 > fdc-test runs 13 tests between qtest_start() and qtest_end(). I thought > that was a nice performance property, and many other tests do the same. > However, if individual startup is okay for the AHCI tester (and you wer= e > actually encouraged to do that), then I guess I could just follow suit. >=20 We can always do it the dumb slow way and add the cached machine hook later if it's too slow. The important thing is the test itself, anyway. I think there are pre and post-test execution hooks you can add to the gtester, so you can throw in a shutdown hook now and then delete it later if you go with the cached machine strategy, and put a single shutdown call before final return from the qtest binary. >> I guess you want some lazy-eval way of spinning up instances only if w= e >> need them, and sharing those instances between tests? Nothing like tha= t >> exists within qtest today, but if you can sort your tests by machine >> type, then: >> >> typedef struct QOSState { >> QTestState *qts; >> + QMachineType type; >> QOSOps *ops; >> } QOSState; >> >> const char *machine_lookup(enum machinetype) >> { >> switch (machinetype) { >> case Q35_2_4: >> return "pc-q35-2.4" >> ... >> } >> } >> >> QOSState *fdc_get_machine(enum machinetype) >> { >> static QOSState *machine; >> >> if (machine && machine->type =3D=3D machinetype) { >> return machine; >> } else { >> if (machine) { >> qtest_pc_shutdown(machine); >> } >> machine =3D qtest_pc_boot("blah blah my defaults here -M %s", >> machine_lookup(machinetype)); >> return machine; >> } >> } >> >> Then just call fdc_get_machine a bunch. You can cram all your defaults >> in fdc_get_machine without bothering the rest of the infrastructure. >=20 > That's exactly what I had in mind, with "share data between tests" -- > but maybe this would lead to my excommunication from qemu-devel :) (Als= o > I think I would not modify QOSState itself.) >=20 Other qtests already do things like this -- but how else are you going to share machines without SOME kind of data sharing? As long as you keep the globals in the qtest itself I think that's fine. No excommunications necessary. Or, if you are truly a purist, you can take a look at create_ahci_io_test in ahci-test.c and its usage of g_test_add_data_func to pass options/data to tests generated in a matrix. >> Not sure if there's a nicer way to do it, I wouldn't lose too much sle= ep >> over design purity in qtests, especially so close to freeze. >> >>> My other question: we're past the 2.4 soft freeze; if I can't fix thi= s >>> until the hard freeze, how big a problem is that? The "new" way won't= be >>> available in 2.4, but the "old" ways should work. >>> >> >> I'm willing to help review and pull things in until fairly late, as lo= ng >> as Peter Maydell doesn't yell at me for doing so. >=20 > Thanks! >=20 >>> (... Oh geez why did I touch the FDC. :() >>> >> >> You're stuck now! >=20 > Thanks for twisting the dagger... I'm swamped by other "more important" > / "more urgent" stuff; I only picked up the "eliminate FDC" thing > because I whined about the FDC, and wanted to avoid an impression that > "Laszlo can only whine and never do something about it". >=20 > I guess I must resist the urge to whine next time. >=20 > Is it perhaps safer to revert those five patches for 2.4, and let me > reboot them (... if I don't change my mind... :)) after 2.4 is out? >=20 > To be clear, this has nothing to do with my willingness, and everything > with my capacity. You may have noticed that I posted the v7 PXB series > yesterday ^W today at 04:40 AM (CEST)... >=20 > Thanks > Laszlo >=20 I'm teasing. Please don't over-commit yourself! Whatever strategy works for your time budget is the one we should do. --js