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

[John Snow]

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.

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

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.

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.

> (... Oh geez why did I touch the FDC. :()
> 

You're stuck now!

> Thanks
> Laszlo
> 

But I'll help.

Thanks,
--js