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

[Laszlo Ersek]

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.

> 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.)

> 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