Re: [Qemu-devel] another isa-fdc problem...

[John Snow <jsnow@redhat.com>]

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 works
>>> fine in the default case (no board-default FDC, which is what we want),
>>> and the traditional option "-drive if=floppy,..." also works as expected.
>>>
>>> However, Ján noticed that on pc-q35-2.4, the "canonical"
>>> frontend/backend option pair
>>>
>>>   -device isa-fdc,driveA=drive-fdc0-0-0 \
>>>   -drive file=...,if=none,id=drive-fdc0-0-0,format=raw
>>>
>>> breaks guest OS detection of the FDC. Markus has now verified that the
>>> qtree looks good (thanks for that), but the guest still doesn't notice
>>> the FDC.
>>>
>>> In <https://bugzilla.redhat.com/show_bug.cgi?id=1227880#c8> and
>>> <https://bugzilla.redhat.com/show_bug.cgi?id=1227880#c9> I wrote:
>>>
>>>> If you create an isa-fdc *outside* of the board init code, ie. outside
>>>> 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 is
>>>> probably what trips up a guest OS -- they look to the CMOS for seeing
>>>> 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 user
>>>> 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 lines
>>> 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.
> 
> You understood right; I didn't dig down this deep. Thanks.
> 
>>> - 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 the
>>> 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 try
>> 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.
> 
> 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 were
> actually encouraged to do that), then I guess I could just follow suit.
> 

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 we
>> need them, and sharing those instances between tests? Nothing like that
>> 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 == machinetype) {
>>         return machine;
>>     } else {
>>         if (machine) {
>>             qtest_pc_shutdown(machine);
>>         }
>>         machine = 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.
> 
> That's exactly what I had in mind, with "share data between tests" --
> but maybe this would lead to my excommunication from qemu-devel :) (Also
> I think I would not modify QOSState itself.)
> 

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 sleep
>> 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 this
>>> 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 long
>> as Peter Maydell doesn't yell at me for doing so.
> 
> Thanks!
> 
>>> (... Oh geez why did I touch the FDC. :()
>>>
>>
>> You're stuck now!
> 
> 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".
> 
> I guess I must resist the urge to whine next time.
> 
> 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?
> 
> 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)...
> 
> Thanks
> Laszlo
> 

I'm teasing. Please don't over-commit yourself! Whatever strategy works
for your time budget is the one we should do.

--js