Re: [Qemu-devel] [RFC v2 0/3] Q35/AHCI -cdrom/-hda desugaring

[Markus Armbruster]

John Snow <address@hidden> writes:

> On 09/19/2014 05:53 AM, Markus Armbruster wrote:
>> John Snow <address@hidden> writes:
>>
>>> This is an extremely rough/quick sketch of
>>> a -cdrom/-hda desugaring fix for Q35/AHCI.
>>>
>>> Before I spent any time on it, I wanted feedback
>>> from Markus or anyone else who had concerns about
>>> how this problem would get fixed.
>>>
>>> This is, then, rough approach #2.
>>>
>>> Highlights:
>>> (1) Add a board property (instead of a HBA property, sigh)
>>>      that defines how we should map (index, (bus,unit)).
>>
>> Imperfect, but it'll do for now.  The place in the boards that sets it
>> should point to the HBA in a comment.
>>
>>> (2) Modify drive_new to accept the MachineClass instead of
>>>      the default interface type. This does not affect how
>>>      default drives get added, because any over-rides to
>>>      the "default type" get handled in options, so while
>>>      it appears we have removed the type of default drives,
>>>      we have not.
>>>
>>> (3) Create helpers for AHCI to assist the Q35 board in
>>>      populating the AHCI device with the IDE drives.
>>>
>>> (4) Create a helper to whine at us for oversights and
>>>      help bug reporters give us more meaningful information.
>>
>> General approach looks good to me; I can see only coding bugs, not
>> design flaws.
>>
>
> I rewrote this series and was about to send it out, but it does fail
> the bios-tables-test because this test uses this command line:
>
> -net none -display none -machine q35,accel=tcg -drive
> file=tests/acpi-test-disk.raw,id=hd0 -device ide-hd,drive=hd0,

qemu: -device ide-hd,drive=hd: Property 'ide-hd.drive' can't take value 'hd', 
it's in use

> Notice it doesn't say if=none for the drive, so after fixing Q35, this
> actually creates a new failure in this test because we will create the
> drive (and device), then fail when trying to create the second device
> attached to the same drive.

Exactly.

Code in question:

    const char *device = "";

    if (!g_strcmp0(data->machine, MACHINE_Q35)) {
        device = ",id=hd -device ide-hd,drive=hd";
    }

    args = g_strdup_printf("-net none -display none %s -drive file=%s%s,",
                           params ? params : "", disk, device);
    qtest_start(args);

Called twice:

1. data->machine = MACHINE_PC (really: "pc")
   params = "-machine accel=tcg"

   Thus, args = "-net none -display none -machine accel=tcg"
   " -drive file=tests/acpi-test-disk.raw,"

   Neither if, bus, unit, nor index specified with -drive, so this asks
   board code to set up an IDE disk (bus, unit) = (0,0), and the board
   code complies.

2. data->machine == MACHINE_Q35 (really: "q35",
   params = "-machine q35,accel=tcg"

   Thus, args = ""-net none -display none -machine q35,accel=tcg"
   " -drive file=tests/acpi-test-disk.raw,id=hd -device ide-hd,drive=hd,"

   Again, this asks board code for IDE disk (0,0).  Before your patch,
   board code silently ignores the request.  Afterwards, it complies.

   Additionally, it sets up another IDE disk (0,0) with device,
   exploiting the misfeature that -device *can* use a drive with if=ide.
   Uses the same drive with two device models, which is unsafe and duly
   rejected.

   The special case for q35 is for coping with the lack of a working
   -drive if=ide.  A working if=ide makes it unnecessary and brings out
   its flaws.

   Even before your patch, the special case is unnecessary: "-drive
   if=none,id=hd,file=... -drive ide-hd,drive=hd" would do for both
   machine types.  I'd make that change in a preliminary patch, because
   that avoids polluting the meat of your patching with correcting test
   bugs.

> I think this test is at fault, but I wanted to be duly diligent and
> ask the question: "Is it a big deal if I break backwards compatibility
> with broken scripts?"

Where "broken scripts" means "our users' broken scripts (if any)".

On the one hand, we've always told users that -device goes with a -drive
if=none.  On the other hand, using a drive not picked up by board code
has always worked anyway.

If we want to remain bug-compatible, we need to make the q35 board code
pick up the IF_IDE drives only for new machine types.  Feasible.  But is
it worthwhile?